diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index 38d376f1..711ab06c 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -1,4 +1,4 @@ """ Open edX Learning ("Learning Core"). """ -__version__ = "0.3.1" +__version__ = "0.3.2" diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index a655cd78..8c5ed764 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -12,12 +12,14 @@ """ from __future__ import annotations -from django.db import transaction -from django.db.models import QuerySet +from django.db import models, transaction +from django.db.models import F, QuerySet, Value +from django.db.models.functions import Coalesce, Concat, Lower from django.utils.translation import gettext as _ from .data import TagDataQuerySet from .models import ObjectTag, Tag, Taxonomy +from .models.utils import ConcatNull # Export this as part of the API TagDoesNotExist = Tag.DoesNotExist @@ -165,8 +167,20 @@ def get_object_tags( filters = {"taxonomy_id": taxonomy_id} if taxonomy_id else {} tags = ( object_tag_class.objects.filter(object_id=object_id, **filters) - .select_related("tag", "taxonomy") - .order_by("id") + # Preload related objects, including data for the "get_lineage" method on ObjectTag/Tag: + .select_related("taxonomy", "tag", "tag__parent", "tag__parent__parent") + # Sort the tags within each taxonomy in "tree order". See Taxonomy._get_filtered_tags_deep for details on this: + .annotate(sort_key=Lower(Concat( + ConcatNull(F("tag__parent__parent__parent__value"), Value("\t")), + ConcatNull(F("tag__parent__parent__value"), Value("\t")), + ConcatNull(F("tag__parent__value"), Value("\t")), + Coalesce(F("tag__value"), F("_value")), + Value("\t"), + output_field=models.CharField(), + ))) + .annotate(taxonomy_name=Coalesce(F("taxonomy__name"), F("_name"))) + # Sort first by taxonomy name, then by tag value in tree order: + .order_by("taxonomy_name", "sort_key") ) return tags diff --git a/openedx_tagging/core/tagging/migrations/0013_tag_parent_blank.py b/openedx_tagging/core/tagging/migrations/0013_tag_parent_blank.py new file mode 100644 index 00000000..d947bbd3 --- /dev/null +++ b/openedx_tagging/core/tagging/migrations/0013_tag_parent_blank.py @@ -0,0 +1,19 @@ +# Generated by Django 3.2.22 on 2023-10-30 21:51 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('oel_tagging', '0012_language_taxonomy'), + ] + + operations = [ + migrations.AlterField( + model_name='tag', + name='parent', + field=models.ForeignKey(blank=True, default=None, help_text='Tag that lives one level up from the current tag, forming a hierarchy.', null=True, on_delete=django.db.models.deletion.CASCADE, related_name='children', to='oel_tagging.tag'), + ), + ] diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index 71318a13..86f94877 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -51,6 +51,7 @@ class Tag(models.Model): parent = models.ForeignKey( "self", null=True, + blank=True, default=None, on_delete=models.CASCADE, related_name="children", @@ -100,18 +101,30 @@ def get_lineage(self) -> Lineage: Queries and returns the lineage of the current tag as a list of Tag.value strings. The root Tag.value is first, followed by its child.value, and on down to self.value. - - Performance note: may perform as many as TAXONOMY_MAX_DEPTH select queries. """ - lineage: Lineage = [] - tag: Tag | None = self - depth = TAXONOMY_MAX_DEPTH - while tag and depth > 0: - lineage.insert(0, tag.value) - tag = tag.parent - depth -= 1 + lineage: Lineage = [self.value] + next_ancestor = self.get_next_ancestor() + while next_ancestor: + lineage.insert(0, next_ancestor.value) + next_ancestor = next_ancestor.get_next_ancestor() return lineage + def get_next_ancestor(self) -> Tag | None: + """ + Fetch the parent of this Tag. + + While doing so, preload several ancestors at the same time, so we can + use fewer database queries than the basic approach of iterating through + parent.parent.parent... + """ + if self.parent_id is None: + return None + if not Tag.parent.is_cached(self): # pylint: disable=no-member + # Parent is not yet loaded. Retrieve our parent, grandparent, and great-grandparent in one query. + # This is not actually changing the parent, just loading it and caching it. + self.parent = Tag.objects.select_related("parent", "parent__parent").get(pk=self.parent_id) + return self.parent + @cached_property def depth(self) -> int: """ @@ -149,6 +162,20 @@ def child_count(self) -> int: return self.taxonomy.tag_set.filter(parent=self).count() return 0 + def clean(self): + """ + Validate this tag before saving + """ + # Don't allow leading or trailing whitespace: + self.value = self.value.strip() + if self.external_id: + self.external_id = self.external_id.strip() + # Don't allow \t (tab) character at all, as we use it for lineage in database queries + if "\t" in self.value: + raise ValidationError("Tags in a taxonomy cannot contain a TAB character.") + if self.external_id and "\t" in self.external_id: + raise ValidationError("Tag external ID cannot contain a TAB character.") + class Taxonomy(models.Model): """ @@ -534,6 +561,7 @@ def add_tag( tag = Tag.objects.create( taxonomy=self, value=tag_value, parent=parent, external_id=external_id ) + tag.full_clean() return tag @@ -802,6 +830,9 @@ def clean(self): raise ValidationError("Invalid _value - empty string") if self.taxonomy and self.taxonomy.name != self._name: raise ValidationError("ObjectTag's _name is out of sync with Taxonomy.name") + if "," in self.object_id or "*" in self.object_id: + # Some APIs may use these characters to allow wildcard matches or multiple matches in the future. + raise ValidationError("Object ID contains invalid characters") def get_lineage(self) -> Lineage: """ diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index 4fe62c72..3281af3f 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -3,6 +3,8 @@ """ from __future__ import annotations +from typing import Any + from rest_framework import serializers from rest_framework.reverse import reverse @@ -61,22 +63,61 @@ class ObjectTagListQueryParamsSerializer(serializers.Serializer): # pylint: dis ) -class ObjectTagSerializer(serializers.ModelSerializer): +class ObjectTagMinimalSerializer(serializers.ModelSerializer): """ - Serializer for the ObjectTag model. + Minimal serializer for the ObjectTag model. """ class Meta: model = ObjectTag - fields = [ + fields = ["value", "lineage"] + + lineage = serializers.ListField(child=serializers.CharField(), source="get_lineage", read_only=True) + + +class ObjectTagSerializer(ObjectTagMinimalSerializer): + """ + Serializer for the ObjectTag model. + """ + class Meta: + model = ObjectTag + fields = ObjectTagMinimalSerializer.Meta.fields + [ + # The taxonomy name "name", - "value", "taxonomy_id", # If the Tag or Taxonomy has been deleted, this ObjectTag shouldn't be shown to users. "is_deleted", ] +class ObjectTagsByTaxonomySerializer(serializers.ModelSerializer): + """ + Serialize a group of ObjectTags, grouped by taxonomy + """ + def to_representation(self, instance: list[ObjectTag]) -> dict: + """ + Convert this list of ObjectTags to the serialized dictionary, grouped by Taxonomy + """ + by_object: dict[str, dict[str, Any]] = {} + for obj_tag in instance: + if obj_tag.object_id not in by_object: + by_object[obj_tag.object_id] = { + "taxonomies": [] + } + taxonomies = by_object[obj_tag.object_id]["taxonomies"] + tax_entry = next((t for t in taxonomies if t["taxonomy_id"] == obj_tag.taxonomy_id), None) + if tax_entry is None: + tax_entry = { + "name": obj_tag.name, + "taxonomy_id": obj_tag.taxonomy_id, + "editable": (not obj_tag.taxonomy.cast().system_defined) if obj_tag.taxonomy else False, + "tags": [] + } + taxonomies.append(tax_entry) + tax_entry["tags"].append(ObjectTagMinimalSerializer(obj_tag).data) + return by_object + + class ObjectTagUpdateBodySerializer(serializers.Serializer): # pylint: disable=abstract-method """ Serializer of the body for the ObjectTag UPDATE view diff --git a/openedx_tagging/core/tagging/rest_api/v1/urls.py b/openedx_tagging/core/tagging/rest_api/v1/urls.py index 72ff87d0..b7841b57 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/urls.py +++ b/openedx_tagging/core/tagging/rest_api/v1/urls.py @@ -10,6 +10,7 @@ router = DefaultRouter() router.register("taxonomies", views.TaxonomyView, basename="taxonomy") router.register("object_tags", views.ObjectTagView, basename="object_tag") +router.register("object_tag_counts", views.ObjectTagCountsView, basename="object_tag_counts") urlpatterns = [ path("", include(router.urls)), diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index 951c8e6f..d2566ca2 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -3,6 +3,8 @@ """ from __future__ import annotations +from typing import Any + from django.db import models from django.http import Http404, HttpResponse from rest_framework import mixins, status @@ -26,12 +28,13 @@ from ...data import TagDataQuerySet from ...import_export.api import export_tags from ...import_export.parsers import ParserFormat -from ...models import Taxonomy +from ...models import ObjectTag, Taxonomy from ...rules import ObjectTagPermissionItem from ..paginators import TAGS_THRESHOLD, DisabledTagsPagination, TagsPagination from .permissions import ObjectTagObjectPermissions, TagObjectPermissions, TaxonomyObjectPermissions from .serializers import ( ObjectTagListQueryParamsSerializer, + ObjectTagsByTaxonomySerializer, ObjectTagSerializer, ObjectTagUpdateBodySerializer, ObjectTagUpdateQueryParamsSerializer, @@ -265,10 +268,6 @@ class ObjectTagView( * 400 - Invalid query parameter * 403 - Permission denied - **Create Query Returns** - * 403 - Permission denied - * 405 - Method not allowed - **Update Parameters** * object_id (required): - The Object ID to add ObjectTags for. @@ -306,20 +305,20 @@ def get_queryset(self) -> models.QuerySet: taxonomy = taxonomy.cast() taxonomy_id = taxonomy.id - perm = "oel_tagging.view_objecttag" - perm_obj = ObjectTagPermissionItem( - taxonomy=taxonomy, - object_id=object_id, - ) - - if not self.request.user.has_perm( - perm, - # The obj arg expects a model, but we are passing an object - perm_obj, # type: ignore[arg-type] - ): - raise PermissionDenied( - "You do not have permission to view object tags for this taxonomy or object_id." - ) + if object_id.endswith("*") or "," in object_id: # pylint: disable=no-else-raise + raise ValidationError("Retrieving tags from multiple objects is not yet supported.") + # Note: This API is actually designed so that in the future it can be extended to return tags for multiple + # objects, e.g. if object_id.endswith("*") then it results in a object_id__startswith query. However, for + # now we have no use case for that so we retrieve tags for one object at a time. + else: + if not self.request.user.has_perm( + "oel_tagging.view_objecttag", + # The obj arg expects a model, but we are passing an object + ObjectTagPermissionItem(taxonomy=taxonomy, object_id=object_id), # type: ignore[arg-type] + ): + raise PermissionDenied( + "You do not have permission to view object tags for this taxonomy or object_id." + ) return get_object_tags(object_id, taxonomy_id) @@ -335,8 +334,13 @@ def retrieve(self, request, *args, **kwargs) -> Response: behavior we want. """ object_tags = self.filter_queryset(self.get_queryset()) - serializer = ObjectTagSerializer(object_tags, many=True) - return Response(serializer.data) + serializer = ObjectTagsByTaxonomySerializer(list(object_tags)) + response_data = serializer.data + if self.kwargs["object_id"] not in response_data: + # For consistency, the key with the object_id should always be present in the response, even if there + # are no tags at all applied to this object. + response_data[self.kwargs["object_id"]] = {"taxonomies": []} + return Response(response_data) def update(self, request, *args, **kwargs) -> Response: """ @@ -405,6 +409,52 @@ def update(self, request, *args, **kwargs) -> Response: return self.retrieve(request, object_id) +@view_auth_classes +class ObjectTagCountsView( + mixins.RetrieveModelMixin, + GenericViewSet, +): + """ + View to retrieve the count of ObjectTags for all matching object IDs. + + This API does NOT bother doing any permission checks as the "# of tags" is not considered sensitive information. + + **Retrieve Parameters** + * object_id_pattern (required): - The Object ID to retrieve ObjectTags for. Can contain '*' at the end + for wildcard matching, or use ',' to separate multiple object IDs. + + **Retrieve Example Requests** + GET api/tagging/v1/object_tag_counts/:object_id_pattern + + **Retrieve Query Returns** + * 200 - Success + """ + + serializer_class = ObjectTagSerializer + lookup_field = "object_id_pattern" + + def retrieve(self, request, *args, **kwargs) -> Response: + """ + Retrieve the counts of object tags that belong to a given object_id pattern + + Note: We override `retrieve` here instead of `list` because we are + passing in the Object ID (object_id) in the path (as opposed to passing + it in as a query_param) to retrieve the ObjectTag counts. + """ + # This API does NOT bother doing any permission checks as the # of tags is not considered sensitive information. + object_id_pattern = self.kwargs["object_id_pattern"] + qs: Any = ObjectTag.objects + if object_id_pattern.endswith("*"): + qs = qs.filter(object_id__startswith=object_id_pattern[0:len(object_id_pattern) - 1]) + elif "*" in object_id_pattern: + raise ValidationError("Wildcard matches are only supported if the * is at the end.") + else: + qs = qs.filter(object_id__in=object_id_pattern.split(",")) + + qs = qs.values("object_id").annotate(num_tags=models.Count("id")).order_by("object_id") + return Response({row["object_id"]: row["num_tags"] for row in qs}) + + @view_auth_classes class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): """ diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 909495a4..a8c2e756 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -85,17 +85,19 @@ def test_get_taxonomies(self) -> None: enabled = list(tagging_api.get_taxonomies()) assert enabled == [ tax1, + self.free_text_taxonomy, tax3, self.language_taxonomy, self.taxonomy, self.system_taxonomy, self.user_taxonomy, - ] + self.dummy_taxonomies + ] assert str(enabled[0]) == f" ({tax1.id}) Enabled" - assert str(enabled[1]) == " (5) Import Taxonomy Test" - assert str(enabled[2]) == " (-1) Languages" - assert str(enabled[3]) == " (1) Life on Earth" - assert str(enabled[4]) == " (4) System defined taxonomy" + 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" with self.assertNumQueries(1): disabled = list(tagging_api.get_taxonomies(enabled=False)) @@ -107,12 +109,13 @@ def test_get_taxonomies(self) -> None: assert both == [ tax2, tax1, + self.free_text_taxonomy, tax3, self.language_taxonomy, self.taxonomy, self.system_taxonomy, self.user_taxonomy, - ] + self.dummy_taxonomies + ] def test_get_tags(self) -> None: assert pretty_format_tags(tagging_api.get_tags(self.taxonomy), parent=False) == [ @@ -263,20 +266,20 @@ def test_resync_object_tags(self) -> None: # At first, none of these will be deleted: assert [(t.value, t.is_deleted) for t in tagging_api.get_object_tags(object_id)] == [ + ("bar", False), + ("foo", False), (self.archaea.value, False), (self.bacteria.value, False), - ("foo", False), - ("bar", False), ] # Delete "bacteria" from the taxonomy: - self.bacteria.delete() # TODO: add an API method for this + tagging_api.delete_tags_from_taxonomy(self.taxonomy, [self.bacteria.value], with_subtags=True) assert [(t.value, t.is_deleted) for t in tagging_api.get_object_tags(object_id)] == [ + ("bar", False), + ("foo", False), (self.archaea.value, False), (self.bacteria.value, True), # <--- deleted! But the value is preserved. - ("foo", False), - ("bar", False), ] # Re-syncing the tags at this point does nothing: @@ -291,10 +294,10 @@ def test_resync_object_tags(self) -> None: # Now the tag is not deleted: assert [(t.value, t.is_deleted) for t in tagging_api.get_object_tags(object_id)] == [ + ("bar", False), + ("foo", False), (self.archaea.value, False), (self.bacteria.value, False), # <--- not deleted - ("foo", False), - ("bar", False), ] # Re-syncing the tags now does nothing: @@ -311,12 +314,12 @@ def test_tag_object(self): self.chordata, ], [ - self.chordata, self.archaebacteria, + self.chordata, ], [ - self.archaebacteria, self.archaea, + self.archaebacteria, ], ] @@ -520,8 +523,9 @@ def test_tag_object_limit(self) -> None: """ Test that the tagging limit is enforced. """ + dummy_taxonomies = self.create_100_taxonomies() # The user can add up to 100 tags to a object - for taxonomy in self.dummy_taxonomies: + for taxonomy in dummy_taxonomies: tagging_api.tag_object( taxonomy, ["Dummy Tag"], @@ -539,7 +543,7 @@ def test_tag_object_limit(self) -> None: assert "Cannot add more than 100 tags to" in str(exc.exception) # Updating existing tags should work - for taxonomy in self.dummy_taxonomies: + for taxonomy in dummy_taxonomies: tagging_api.tag_object( taxonomy, ["New Dummy Tag"], @@ -547,7 +551,7 @@ def test_tag_object_limit(self) -> None: ) # Updating existing tags adding a new one should fail - for taxonomy in self.dummy_taxonomies: + for taxonomy in dummy_taxonomies: with self.assertRaises(ValueError) as exc: tagging_api.tag_object( taxonomy, @@ -558,15 +562,16 @@ def test_tag_object_limit(self) -> None: assert "Cannot add more than 100 tags to" in str(exc.exception) def test_get_object_tags(self) -> None: - # Alpha tag has no taxonomy + # Alpha tag has no taxonomy (as if the taxonomy had been deleted) alpha = ObjectTag(object_id="abc") alpha.name = self.taxonomy.name - alpha.value = self.mammalia.value + alpha.value = "alpha" alpha.save() # Beta tag has a closed taxonomy beta = ObjectTag.objects.create( object_id="abc", taxonomy=self.taxonomy, + tag=self.taxonomy.tag_set.get(value="Protista"), ) # Fetch all the tags for a given object ID diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index 8c006a14..88bfeaad 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -33,12 +33,14 @@ class TestTagTaxonomyMixin: 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.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.free_text_taxonomy = Taxonomy.objects.create(name="Free Text", allow_free_text=True) + + # References to some tags: self.archaea = get_tag("Archaea") self.archaebacteria = get_tag("Archaebacteria") self.bacteria = get_tag("Bacteria") @@ -73,7 +75,41 @@ def setUp(self): get_tag("System Tag 4"), ] - self.dummy_taxonomies = [] + def create_sort_test_taxonomy(self) -> Taxonomy: + """ + Helper method to create a taxonomy that's difficult to sort correctly in tree order. + """ + # pylint: disable=unused-variable + taxonomy = api.create_taxonomy("Sort Test") + + root1 = Tag.objects.create(taxonomy=taxonomy, value="1") + child1_1 = Tag.objects.create(taxonomy=taxonomy, value="11", parent=root1) + child1_2 = Tag.objects.create(taxonomy=taxonomy, value="2", parent=root1) + child1_3 = Tag.objects.create(taxonomy=taxonomy, value="1 A", parent=root1) + child1_4 = Tag.objects.create(taxonomy=taxonomy, value="11111", parent=root1) + grandchild1_4_1 = Tag.objects.create(taxonomy=taxonomy, value="1111-grandchild", parent=child1_4) + + root2 = Tag.objects.create(taxonomy=taxonomy, value="111") + child2_1 = Tag.objects.create(taxonomy=taxonomy, value="11111111", parent=root2) + child2_2 = Tag.objects.create(taxonomy=taxonomy, value="123", parent=root2) + + root1 = Tag.objects.create(taxonomy=taxonomy, value="ALPHABET") + child1_1 = Tag.objects.create(taxonomy=taxonomy, value="Android", parent=root1) + child1_2 = Tag.objects.create(taxonomy=taxonomy, value="abacus", parent=root1) + child1_2 = Tag.objects.create(taxonomy=taxonomy, value="azure", parent=root1) + child1_3 = Tag.objects.create(taxonomy=taxonomy, value="aardvark", parent=root1) + child1_4 = Tag.objects.create(taxonomy=taxonomy, value="ANVIL", parent=root1) + + root2 = Tag.objects.create(taxonomy=taxonomy, value="abstract") + child2_1 = Tag.objects.create(taxonomy=taxonomy, value="Andes", parent=root2) + child2_2 = Tag.objects.create(taxonomy=taxonomy, value="azores islands", parent=root2) + return taxonomy + + def create_100_taxonomies(self): + """ + Helper method to create 100 taxonomies and to apply a tag from each to an object + """ + dummy_taxonomies = [] for i in range(100): taxonomy = Taxonomy.objects.create( name=f"ZZ Dummy Taxonomy {i:03}", @@ -86,7 +122,8 @@ def setUp(self): _name=taxonomy.name, _value="Dummy Tag", ) - self.dummy_taxonomies.append(taxonomy) + dummy_taxonomies.append(taxonomy) + return dummy_taxonomies class TaxonomyTestSubclassA(Taxonomy): @@ -203,13 +240,34 @@ def test_unique_tags(self): ("eubacteria", ["Bacteria", "Eubacteria"]), # Third level tags return three levels ("chordata", ["Eukaryota", "Animalia", "Chordata"]), - # Lineage beyond TAXONOMY_MAX_DEPTH won't trace back to the root - ("mammalia", ["Animalia", "Chordata", "Mammalia"]), + # Even fourth level tags work + ("mammalia", ["Eukaryota", "Animalia", "Chordata", "Mammalia"]), ) @ddt.unpack def test_get_lineage(self, tag_attr, lineage): assert getattr(self, tag_attr).get_lineage() == lineage + def test_trailing_whitespace(self): + """ + Test that tags automatically strip out trailing/leading whitespace + """ + t = self.taxonomy.add_tag(" white space ") + assert t.value == "white space" + # And via the API: + t2 = api.add_tag_to_taxonomy(self.taxonomy, "\t value\n") + assert t2.value == "value" + + def test_no_tab(self): + """ + Test that tags cannot contain a TAB character, which we use as a field + separator in the database when computing lineage. + """ + with pytest.raises(ValidationError): + self.taxonomy.add_tag("has\ttab") + # And via the API: + with pytest.raises(ValidationError): + api.add_tag_to_taxonomy(self.taxonomy, "first\tsecond") + @ddt.ddt class TestFilteredTagsClosedTaxonomy(TestTagTaxonomyMixin, TestCase): @@ -444,22 +502,13 @@ def test_usage_count(self) -> None: "Bacteria (None) (used: 3, children: 2)", ] - def test_pathological_tree_sort(self) -> None: + def test_tree_sort(self) -> None: """ - Check for bugs in how tree sorting happens, if the tag names are very - similar. + Verify that taxonomies can be sorted correctly in tree orer (case insensitive). + + The taxonomy used contains values that are tricky to sort correctly unless the tree sort algorithm is correct. """ - # pylint: disable=unused-variable - taxonomy = api.create_taxonomy("Sort Test") - root1 = Tag.objects.create(taxonomy=taxonomy, value="1") - child1_1 = Tag.objects.create(taxonomy=taxonomy, value="11", parent=root1) - child1_2 = Tag.objects.create(taxonomy=taxonomy, value="2", parent=root1) - child1_3 = Tag.objects.create(taxonomy=taxonomy, value="1 A", parent=root1) - child1_4 = Tag.objects.create(taxonomy=taxonomy, value="11111", parent=root1) - grandchild1_4_1 = Tag.objects.create(taxonomy=taxonomy, value="1111-grandchild", parent=child1_4) - root2 = Tag.objects.create(taxonomy=taxonomy, value="111") - child2_1 = Tag.objects.create(taxonomy=taxonomy, value="11111111", parent=root2) - child2_2 = Tag.objects.create(taxonomy=taxonomy, value="123", parent=root2) + taxonomy = self.create_sort_test_taxonomy() result = pretty_format_tags(taxonomy.get_filtered_tags()) assert result == [ "1 (None) (children: 4)", @@ -471,27 +520,6 @@ def test_pathological_tree_sort(self) -> None: "111 (None) (children: 2)", " 11111111 (111) (children: 0)", " 123 (111) (children: 0)", - ] - - def test_case_insensitive_sort(self) -> None: - """ - Make sure the sorting is case-insensitive - """ - # pylint: disable=unused-variable - taxonomy = api.create_taxonomy("Sort Test") - root1 = Tag.objects.create(taxonomy=taxonomy, value="ALPHABET") - child1_1 = Tag.objects.create(taxonomy=taxonomy, value="Android", parent=root1) - child1_2 = Tag.objects.create(taxonomy=taxonomy, value="abacus", parent=root1) - child1_2 = Tag.objects.create(taxonomy=taxonomy, value="azure", parent=root1) - child1_3 = Tag.objects.create(taxonomy=taxonomy, value="aardvark", parent=root1) - child1_4 = Tag.objects.create(taxonomy=taxonomy, value="ANVIL", parent=root1) - - root2 = Tag.objects.create(taxonomy=taxonomy, value="abstract") - child2_1 = Tag.objects.create(taxonomy=taxonomy, value="Andes", parent=root2) - child2_2 = Tag.objects.create(taxonomy=taxonomy, value="azores islands", parent=root2) - - result = pretty_format_tags(taxonomy.get_filtered_tags()) - assert result == [ "abstract (None) (children: 2)", " Andes (abstract) (children: 0)", " azores islands (abstract) (children: 0)", @@ -503,16 +531,6 @@ def test_case_insensitive_sort(self) -> None: " azure (ALPHABET) (children: 0)", ] - # And it's case insensitive when getting only a single level: - result = pretty_format_tags(taxonomy.get_filtered_tags(parent_tag_value="ALPHABET", depth=1)) - assert result == [ - " aardvark (ALPHABET) (children: 0)", - " abacus (ALPHABET) (children: 0)", - " Android (ALPHABET) (children: 0)", - " ANVIL (ALPHABET) (children: 0)", - " azure (ALPHABET) (children: 0)", - ] - class TestFilteredTagsFreeTextTaxonomy(TestCase): """ @@ -669,16 +687,13 @@ def test_object_tag_lineage(self): assert object_tag.get_lineage() == ["Another tag"] def test_validate_value_free_text(self): - open_taxonomy = Taxonomy.objects.create( - name="Freetext Life", - allow_free_text=True, - ) + assert self.free_text_taxonomy.allow_free_text # An empty string or other non-string is not valid in a free-text taxonomy - assert open_taxonomy.validate_value("") is False - assert open_taxonomy.validate_value(None) is False - assert open_taxonomy.validate_value(True) is False + assert self.free_text_taxonomy.validate_value("") is False + assert self.free_text_taxonomy.validate_value(None) is False + assert self.free_text_taxonomy.validate_value(True) is False # But any other string value is valid: - assert open_taxonomy.validate_value("Any text we want") is True + assert self.free_text_taxonomy.validate_value("Any text we want") is True def test_validate_value_closed(self): """ @@ -730,43 +745,53 @@ def test_tag_case(self) -> None: tag=self.chordata, ).save() + def test_invalid_id(self): + """ + Test attempting to create object tags with invalid characters in the object ID + """ + args = {"tags": ["test"], "taxonomy": self.free_text_taxonomy} + with pytest.raises(ValidationError): + api.tag_object(object_id="wildcard*", **args) + with pytest.raises(ValidationError): + api.tag_object(object_id="one,two,three", **args) + api.tag_object(object_id="valid", **args) + def test_is_deleted(self): self.taxonomy.allow_multiple = True self.taxonomy.save() - open_taxonomy = Taxonomy.objects.create(name="Freetext Life", allow_free_text=True, allow_multiple=True) object_id = "obj1" # Create some tags: api.tag_object(self.taxonomy, [self.archaea.value, self.bacteria.value], object_id) # Regular tags - api.tag_object(open_taxonomy, ["foo", "bar", "tribble"], object_id) # Free text tags + api.tag_object(self.free_text_taxonomy, ["foo", "bar", "tribble"], object_id) # Free text tags # At first, none of these will be deleted: assert [(t.value, t.is_deleted) for t in api.get_object_tags(object_id)] == [ - (self.archaea.value, False), - (self.bacteria.value, False), - ("foo", False), ("bar", False), + ("foo", False), ("tribble", False), + (self.archaea.value, False), + (self.bacteria.value, False), ] # Delete "bacteria" from the taxonomy: - self.bacteria.delete() # TODO: add an API method for this + api.delete_tags_from_taxonomy(self.taxonomy, ["Bacteria"], with_subtags=True) assert [(t.value, t.is_deleted) for t in api.get_object_tags(object_id)] == [ - (self.archaea.value, False), - (self.bacteria.value, True), # <--- deleted! But the value is preserved. - ("foo", False), ("bar", False), + ("foo", False), ("tribble", False), + (self.archaea.value, False), + (self.bacteria.value, True), # <--- deleted! But the value is preserved. ] # Then delete the whole free text taxonomy - open_taxonomy.delete() + self.free_text_taxonomy.delete() assert [(t.value, t.is_deleted) for t in api.get_object_tags(object_id)] == [ - (self.archaea.value, False), - (self.bacteria.value, True), # <--- deleted! But the value is preserved. - ("foo", True), # <--- Deleted, but the value is preserved ("bar", True), # <--- Deleted, but the value is preserved + ("foo", True), # <--- Deleted, but the value is preserved ("tribble", True), # <--- Deleted, but the value is preserved + (self.archaea.value, False), + (self.bacteria.value, True), # <--- deleted! But the value is preserved. ] diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 6a312546..bdb214ae 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -12,6 +12,7 @@ from rest_framework import status from rest_framework.test import APITestCase +from openedx_tagging.core.tagging import api from openedx_tagging.core.tagging.import_export import api as import_export_api from openedx_tagging.core.tagging.import_export.parsers import ParserFormat from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy @@ -19,6 +20,7 @@ from openedx_tagging.core.tagging.rest_api.paginators import TagsPagination from openedx_tagging.core.tagging.rules import can_change_object_tag_objectid, can_view_object_tag_objectid +from .test_models import TestTagTaxonomyMixin from .utils import pretty_format_tags User = get_user_model() @@ -30,6 +32,7 @@ OBJECT_TAGS_RETRIEVE_URL = "/tagging/rest_api/v1/object_tags/{object_id}/" +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}/?taxonomy={taxonomy_id}" LANGUAGE_TAXONOMY_ID = -1 @@ -100,9 +103,9 @@ class TestTaxonomyViewSet(TestTaxonomyViewMixin): ) @ddt.unpack def test_list_taxonomy_queryparams(self, enabled, expected_status: int, expected_count: int | None): - Taxonomy.objects.create(name="Taxonomy enabled 1", enabled=True).save() - Taxonomy.objects.create(name="Taxonomy enabled 2", enabled=True).save() - Taxonomy.objects.create(name="Taxonomy disabled", enabled=False).save() + api.create_taxonomy(name="Taxonomy enabled 1", enabled=True) + api.create_taxonomy(name="Taxonomy enabled 2", enabled=True) + api.create_taxonomy(name="Taxonomy disabled", enabled=False) url = TAXONOMY_LIST_URL @@ -136,11 +139,11 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int): def test_list_taxonomy_pagination(self) -> None: url = TAXONOMY_LIST_URL - Taxonomy.objects.create(name="T1", enabled=True).save() - Taxonomy.objects.create(name="T2", enabled=True).save() - Taxonomy.objects.create(name="T3", enabled=False).save() - Taxonomy.objects.create(name="T4", enabled=False).save() - Taxonomy.objects.create(name="T5", enabled=False).save() + api.create_taxonomy(name="T1", enabled=True) + api.create_taxonomy(name="T2", enabled=True) + api.create_taxonomy(name="T3", enabled=False) + api.create_taxonomy(name="T4", enabled=False) + api.create_taxonomy(name="T5", enabled=False) self.client.force_authenticate(user=self.staff) @@ -177,7 +180,7 @@ def test_list_invalid_page(self) -> None: @ddt.unpack def test_detail_taxonomy(self, user_attr: str | None, taxonomy_data: dict[str, bool], expected_status: int): create_data = {"name": "taxonomy detail test", **taxonomy_data} - taxonomy = Taxonomy.objects.create(**create_data) + taxonomy = api.create_taxonomy(**create_data) # type: ignore[arg-type] url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) if user_attr: @@ -265,12 +268,11 @@ def test_create_taxonomy_system_defined(self, create_data): ) @ddt.unpack def test_update_taxonomy(self, user_attr, expected_status): - taxonomy = Taxonomy.objects.create( + taxonomy = api.create_taxonomy( name="test update taxonomy", description="taxonomy description", enabled=True, ) - taxonomy.save() url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) @@ -303,10 +305,10 @@ def test_update_taxonomy_system_defined(self, system_defined, expected_status): """ Test that we can't update system_defined field """ - taxonomy = Taxonomy.objects.create(name="test system taxonomy") - if system_defined: - taxonomy.taxonomy_class = SystemDefinedTaxonomy - taxonomy.save() + taxonomy = api.create_taxonomy( + name="test system taxonomy", + taxonomy_class=SystemDefinedTaxonomy if system_defined else None, + ) url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) self.client.force_authenticate(user=self.staff) @@ -327,8 +329,7 @@ def test_update_taxonomy_404(self): ) @ddt.unpack def test_patch_taxonomy(self, user_attr, expected_status): - taxonomy = Taxonomy.objects.create(name="test patch taxonomy", enabled=False) - taxonomy.save() + taxonomy = api.create_taxonomy(name="test patch taxonomy", enabled=False) url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) @@ -360,10 +361,10 @@ def test_patch_taxonomy_system_defined(self, system_defined, expected_status): """ Test that we can't patch system_defined field """ - taxonomy = Taxonomy.objects.create(name="test system taxonomy") - if system_defined: - taxonomy.taxonomy_class = SystemDefinedTaxonomy - taxonomy.save() + taxonomy = api.create_taxonomy( + name="test system taxonomy", + taxonomy_class=SystemDefinedTaxonomy if system_defined else None, + ) url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) self.client.force_authenticate(user=self.staff) @@ -384,8 +385,7 @@ def test_patch_taxonomy_404(self): ) @ddt.unpack def test_delete_taxonomy(self, user_attr, expected_status): - taxonomy = Taxonomy.objects.create(name="test delete taxonomy") - taxonomy.save() + taxonomy = api.create_taxonomy(name="test delete taxonomy") url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) @@ -417,8 +417,7 @@ def test_export_taxonomy(self, output_format, content_type): """ Tests if a user can export a taxonomy """ - taxonomy = Taxonomy.objects.create(name="T1", enabled=True) - taxonomy.save() + taxonomy = api.create_taxonomy(name="T1") for i in range(20): # Valid ObjectTags Tag.objects.create(taxonomy=taxonomy, value=f"Tag {i}").save() @@ -445,11 +444,9 @@ def test_export_taxonomy_download(self, output_format, content_type): """ Tests if a user can export a taxonomy with download option """ - taxonomy = Taxonomy.objects.create(name="T1", enabled=True) - taxonomy.save() + taxonomy = api.create_taxonomy(name="T1") for i in range(20): - # Valid ObjectTags - Tag.objects.create(taxonomy=taxonomy, value=f"Tag {i}").save() + api.add_tag_to_taxonomy(taxonomy=taxonomy, tag=f"Tag {i}") url = TAXONOMY_EXPORT_URL.format(pk=taxonomy.pk) @@ -469,8 +466,7 @@ def test_export_taxonomy_invalid_param_output_format(self): """ Tests if a user can export a taxonomy using an invalid output_format param """ - taxonomy = Taxonomy.objects.create(name="T1", enabled=True) - taxonomy.save() + taxonomy = api.create_taxonomy(name="T1") url = TAXONOMY_EXPORT_URL.format(pk=taxonomy.pk) @@ -482,8 +478,7 @@ def test_export_taxonomy_invalid_param_download(self): """ Tests if a user can export a taxonomy using an invalid output_format param """ - taxonomy = Taxonomy.objects.create(name="T1", enabled=True) - taxonomy.save() + taxonomy = api.create_taxonomy(name="T1") url = TAXONOMY_EXPORT_URL.format(pk=taxonomy.pk) @@ -496,8 +491,7 @@ def test_export_taxonomy_unauthorized(self): Tests if a user can export a taxonomy that he doesn't have authorization """ # Only staff can view a disabled taxonomy - taxonomy = Taxonomy.objects.create(name="T1", enabled=False) - taxonomy.save() + taxonomy = api.create_taxonomy(name="T1", enabled=False) url = TAXONOMY_EXPORT_URL.format(pk=taxonomy.pk) @@ -509,12 +503,13 @@ def test_export_taxonomy_unauthorized(self): @ddt.ddt -class TestObjectTagViewSet(APITestCase): +class TestObjectTagViewSet(TestTagTaxonomyMixin, APITestCase): """ Testing various cases for the ObjectTagView. """ def setUp(self): + super().setUp() def _change_object_permission(user, object_id: str) -> bool: """ @@ -534,88 +529,34 @@ def _view_object_permission(user, object_id: str) -> bool: return can_view_object_tag_objectid(user, object_id) - super().setUp() - - self.user = User.objects.create( - username="user", - email="user@example.com", - ) - - self.staff = User.objects.create( - username="staff", - email="staff@example.com", - is_staff=True, - ) - - # System-defined language taxonomy with valid ObjectTag - self.system_taxonomy = SystemDefinedTaxonomy.objects.create(name="System Taxonomy") - self.tag1 = Tag.objects.create(taxonomy=self.system_taxonomy, value="Tag 1") - ObjectTag.objects.create(object_id="abc", taxonomy=self.system_taxonomy, tag=self.tag1) - - # Language system-defined language taxonomy - self.language_taxonomy = Taxonomy.objects.get(pk=LANGUAGE_TAXONOMY_ID) - - # Closed Taxonomies created by taxonomy admins, each with 20 ObjectTags - self.enabled_taxonomy = Taxonomy.objects.create(name="Enabled Taxonomy", allow_multiple=False) - self.disabled_taxonomy = Taxonomy.objects.create(name="Disabled Taxonomy", enabled=False, allow_multiple=False) - self.multiple_taxonomy = Taxonomy.objects.create(name="Multiple Taxonomy", allow_multiple=True) - for i in range(20): - # Valid ObjectTags - tag_enabled = Tag.objects.create(taxonomy=self.enabled_taxonomy, value=f"Tag {i}") - tag_disabled = Tag.objects.create(taxonomy=self.disabled_taxonomy, value=f"Tag {i}") - tag_multiple = Tag.objects.create(taxonomy=self.multiple_taxonomy, value=f"Tag {i}") - ObjectTag.objects.create( - object_id="abc", taxonomy=self.enabled_taxonomy, tag=tag_enabled, _value=tag_enabled.value - ) - ObjectTag.objects.create( - object_id="abc", taxonomy=self.disabled_taxonomy, tag=tag_disabled, _value=tag_disabled.value - ) - ObjectTag.objects.create( - object_id="abc", taxonomy=self.multiple_taxonomy, tag=tag_multiple, _value=tag_multiple.value - ) - - # Free-Text Taxonomies created by taxonomy admins, each linked - # to 10 ObjectTags - self.open_taxonomy_enabled = Taxonomy.objects.create( - name="Enabled Free-Text Taxonomy", allow_free_text=True, allow_multiple=False, - ) - self.open_taxonomy_disabled = Taxonomy.objects.create( - name="Disabled Free-Text Taxonomy", allow_free_text=True, enabled=False, allow_multiple=False, - ) - for i in range(10): - ObjectTag.objects.create(object_id="abc", taxonomy=self.open_taxonomy_enabled, _value=f"Free Text {i}") - ObjectTag.objects.create(object_id="abc", taxonomy=self.open_taxonomy_disabled, _value=f"Free Text {i}") - - self.dummy_taxonomies = [] - for i in range(100): - taxonomy = Taxonomy.objects.create( - name=f"Dummy Taxonomy {i}", - allow_free_text=True, - allow_multiple=True - ) - ObjectTag.objects.create( - object_id="limit_tag_count", - taxonomy=taxonomy, - _name=taxonomy.name, - _value="Dummy Tag" - ) - self.dummy_taxonomies.append(taxonomy) - # Override the object permission for the test rules.set_perm("oel_tagging.change_objecttag_objectid", _change_object_permission) rules.set_perm("oel_tagging.view_objecttag_objectid", _view_object_permission) + # Create a staff user: + self.staff = User.objects.create(username="staff", email="staff@example.com", is_staff=True) + + # For this test, allow multiple "Life on Earth" tags: + self.taxonomy.allow_multiple = True + self.taxonomy.save() + @ddt.data( - (None, status.HTTP_401_UNAUTHORIZED, None), - ("user", status.HTTP_200_OK, 81), - ("staff", status.HTTP_200_OK, 81), + (None, status.HTTP_401_UNAUTHORIZED), + ("user_1", status.HTTP_200_OK), + ("staff", status.HTTP_200_OK), ) @ddt.unpack - def test_retrieve_object_tags(self, user_attr, expected_status, expected_count): + def test_retrieve_object_tags(self, user_attr, expected_status): """ Test retrieving object tags """ - url = OBJECT_TAGS_RETRIEVE_URL.format(object_id="abc") + object_id = "problem15" + + # 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]) + + url = OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id) if user_attr: user = getattr(self, user_attr) @@ -625,7 +566,126 @@ def test_retrieve_object_tags(self, user_attr, expected_status, expected_count): assert response.status_code == expected_status if status.is_success(expected_status): - assert len(response.data) == expected_count + # Check the response, first converting from OrderedDict to regular dicts for simplicity. + assert response.data == { + # In the future, this API may allow retrieving tags for multiple objects at once, so it's grouped by + # object ID. + "problem15": { + "taxonomies": [ + { + "name": "Life on Earth", + "taxonomy_id": 1, + "editable": True, + "tags": [ + # Note: based on tree order (Animalia before Fungi), this tag comes first even though it + # starts with "M" and Fungi starts with "F" + { + "value": "Mammalia", + "lineage": ["Eukaryota", "Animalia", "Chordata", "Mammalia"], + }, + { + "value": "Fungi", + "lineage": ["Eukaryota", "Fungi"], + }, + ] + }, + { + "name": "User Authors", + "taxonomy_id": 3, + "editable": False, + "tags": [ + { + "value": "test_user_1", + "lineage": ["test_user_1"], + }, + ], + } + ], + }, + } + + def prepare_for_sort_test(self) -> tuple[str, list[dict]]: + """ + Tag an object with tags from the "sort test" taxonomy + """ + object_id = "problem7" + # Some selected tags to use, from the taxonomy create by self.create_sort_test_taxonomy() + sort_test_tags = [ + "ANVIL", + "Android", + "azores islands", + "abstract", + "11111111", + "111", + "123", + "1 A", + "1111-grandchild", + ] + + # Apply the object tags: + taxonomy = self.create_sort_test_taxonomy() + api.tag_object(object_id=object_id, taxonomy=taxonomy, tags=sort_test_tags) + + # The result we expect to see when retrieving the object tags, after applying the list above. + # Note: the full taxonomy looks like the following, so this is the order we + # expect, although not all of these tags were included. + # 1 + # 1 A + # 11 + # 11111 + # 1111-grandchild + # 2 + # 111 + # 11111111 + # 123 + # abstract + # Andes + # azores islands + # ALPHABET + # aardvark + # abacus + # Android + # ANVIL + # azure + sort_test_applied_result = [ + {"value": "1 A", "lineage": ["1", "1 A"]}, + {"value": "1111-grandchild", "lineage": ["1", "11111", "1111-grandchild"]}, + {"value": "111", "lineage": ["111"]}, + {"value": "11111111", "lineage": ["111", "11111111"]}, + {"value": "123", "lineage": ["111", "123"]}, + {"value": "abstract", "lineage": ["abstract"]}, + {"value": "azores islands", "lineage": ["abstract", "azores islands"]}, + {"value": "Android", "lineage": ["ALPHABET", "Android"]}, + {"value": "ANVIL", "lineage": ["ALPHABET", "ANVIL"]}, + ] + return object_id, sort_test_applied_result + + def test_retrieve_object_tags_sorted(self): + """ + Test the sort order of the object tags retrieved from the get object + tags API. + """ + object_id, sort_test_applied_result = self.prepare_for_sort_test() + + url = OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id) + self.client.force_authenticate(user=self.user_1) + response = self.client.get(url) + assert response.status_code == 200 + assert response.data[object_id]["taxonomies"][0]["name"] == "Sort Test" + assert response.data[object_id]["taxonomies"][0]["tags"] == sort_test_applied_result + + def test_retrieve_object_tags_query_count(self): + """ + Test how many queries are used when retrieving object tags + """ + object_id, sort_test_applied_result = self.prepare_for_sort_test() + + url = OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id) + self.client.force_authenticate(user=self.user_1) + with self.assertNumQueries(1): + response = self.client.get(url) + assert response.status_code == 200 + assert response.data[object_id]["taxonomies"][0]["tags"] == sort_test_applied_result def test_retrieve_object_tags_unauthorized(self): """ @@ -637,34 +697,56 @@ def test_retrieve_object_tags_unauthorized(self): assert response.status_code == status.HTTP_403_FORBIDDEN @ddt.data( - (None, "abc", status.HTTP_401_UNAUTHORIZED, None), - ("user", "abc", status.HTTP_200_OK, 20), - ("staff", "abc", status.HTTP_200_OK, 20), + (None, status.HTTP_401_UNAUTHORIZED), + ("user_1", status.HTTP_200_OK), + ("staff", status.HTTP_200_OK), ) @ddt.unpack def test_retrieve_object_tags_taxonomy_queryparam( - self, user_attr, object_id, expected_status, expected_count + self, user_attr, expected_status, ): """ Test retrieving object tags for specific taxonomies provided """ + object_id = "html7" + + # 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]) + url = OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id) if user_attr: user = getattr(self, user_attr) self.client.force_authenticate(user=user) - response = self.client.get(url, {"taxonomy": self.enabled_taxonomy.pk}) + response = self.client.get(url, {"taxonomy": self.user_taxonomy.pk}) assert response.status_code == expected_status if status.is_success(expected_status): - assert len(response.data) == expected_count - for object_tag in response.data: - assert object_tag.get("is_deleted") is False - assert object_tag.get("taxonomy_id") == self.enabled_taxonomy.pk + assert response.data == { + # In the future, this API may allow retrieving tags for multiple objects at once, so it's grouped by + # object ID. + object_id: { + "taxonomies": [ + # The "Life on Earth" tags are excluded here... + { + "name": "User Authors", + "taxonomy_id": 3, + "editable": False, + "tags": [ + { + "value": "test_user_1", + "lineage": ["test_user_1"], + }, + ], + } + ], + }, + } @ddt.data( (None, "abc", status.HTTP_401_UNAUTHORIZED), - ("user", "abc", status.HTTP_400_BAD_REQUEST), + ("user_1", "abc", status.HTTP_400_BAD_REQUEST), ("staff", "abc", status.HTTP_400_BAD_REQUEST), ) @ddt.unpack @@ -686,9 +768,9 @@ def test_retrieve_object_tags_invalid_taxonomy_queryparam(self, user_attr, objec (None, "POST", status.HTTP_401_UNAUTHORIZED), (None, "PATCH", status.HTTP_401_UNAUTHORIZED), (None, "DELETE", status.HTTP_401_UNAUTHORIZED), - ("user", "POST", status.HTTP_405_METHOD_NOT_ALLOWED), - ("user", "PATCH", status.HTTP_405_METHOD_NOT_ALLOWED), - ("user", "DELETE", status.HTTP_405_METHOD_NOT_ALLOWED), + ("user_1", "POST", status.HTTP_405_METHOD_NOT_ALLOWED), + ("user_1", "PATCH", status.HTTP_405_METHOD_NOT_ALLOWED), + ("user_1", "DELETE", status.HTTP_405_METHOD_NOT_ALLOWED), ("staff", "POST", status.HTTP_405_METHOD_NOT_ALLOWED), ("staff", "PATCH", status.HTTP_405_METHOD_NOT_ALLOWED), ("staff", "DELETE", status.HTTP_405_METHOD_NOT_ALLOWED), @@ -724,148 +806,102 @@ def test_object_tags_remaining_http_methods( @ddt.data( # Users and staff can add tags - (None, "language_taxonomy", ["Portuguese"], status.HTTP_401_UNAUTHORIZED), - ("user", "language_taxonomy", ["Portuguese"], status.HTTP_200_OK), - ("staff", "language_taxonomy", ["Portuguese"], status.HTTP_200_OK), - # Users and staff can clear add tags - (None, "enabled_taxonomy", ["Tag 1"], status.HTTP_401_UNAUTHORIZED), - ("user", "enabled_taxonomy", ["Tag 1"], status.HTTP_200_OK), - ("staff", "enabled_taxonomy", ["Tag 1"], status.HTTP_200_OK), + (None, "language_taxonomy", {}, ["Portuguese"], status.HTTP_401_UNAUTHORIZED), + ("user_1", "language_taxonomy", {}, ["Portuguese"], status.HTTP_200_OK), + ("staff", "language_taxonomy", {}, ["Portuguese"], status.HTTP_200_OK), + # user_1s and staff can clear add tags + (None, "taxonomy", {}, ["Fungi"], status.HTTP_401_UNAUTHORIZED), + ("user_1", "taxonomy", {}, ["Fungi"], status.HTTP_200_OK), + ("staff", "taxonomy", {}, ["Fungi"], status.HTTP_200_OK), # Nobody can add tag using a disabled taxonomy - (None, "disabled_taxonomy", ["Tag 1"], status.HTTP_401_UNAUTHORIZED), - ("user", "disabled_taxonomy", ["Tag 1"], status.HTTP_403_FORBIDDEN), - ("staff", "disabled_taxonomy", ["Tag 1"], status.HTTP_403_FORBIDDEN), - # Users and staff can add a single tag using a allow_multiple=True taxonomy - (None, "multiple_taxonomy", ["Tag 1"], status.HTTP_401_UNAUTHORIZED), - ("user", "multiple_taxonomy", ["Tag 1"], status.HTTP_200_OK), - ("staff", "multiple_taxonomy", ["Tag 1"], status.HTTP_200_OK), - # Users and staff can add tags using an open taxonomy - (None, "open_taxonomy_enabled", ["tag1"], status.HTTP_401_UNAUTHORIZED), - ("user", "open_taxonomy_enabled", ["tag1"], status.HTTP_200_OK), - ("staff", "open_taxonomy_enabled", ["tag1"], status.HTTP_200_OK), + (None, "taxonomy", {"enabled": False}, ["Fungi"], status.HTTP_401_UNAUTHORIZED), + ("user_1", "taxonomy", {"enabled": False}, ["Fungi"], status.HTTP_403_FORBIDDEN), + ("staff", "taxonomy", {"enabled": False}, ["Fungi"], status.HTTP_403_FORBIDDEN), + # If allow_multiple=True, multiple tags can be added, but not if it's false: + ("user_1", "taxonomy", {"allow_multiple": True}, ["Mammalia", "Fungi"], status.HTTP_200_OK), + ("user_1", "taxonomy", {"allow_multiple": False}, ["Mammalia", "Fungi"], status.HTTP_400_BAD_REQUEST), + # user_1s and staff can add tags using an open taxonomy + (None, "free_text_taxonomy", {}, ["tag1"], status.HTTP_401_UNAUTHORIZED), + ("user_1", "free_text_taxonomy", {}, ["tag1", "tag2"], status.HTTP_200_OK), + ("staff", "free_text_taxonomy", {}, ["tag1", "tag4"], status.HTTP_200_OK), # Nobody can add tags using a disabled open taxonomy - (None, "open_taxonomy_disabled", ["tag1"], status.HTTP_401_UNAUTHORIZED), - ("user", "open_taxonomy_disabled", ["tag1"], status.HTTP_403_FORBIDDEN), - ("staff", "open_taxonomy_disabled", ["tag1"], status.HTTP_403_FORBIDDEN), + (None, "free_text_taxonomy", {"enabled": False}, ["tag1"], status.HTTP_401_UNAUTHORIZED), + ("user_1", "free_text_taxonomy", {"enabled": False}, ["tag1"], status.HTTP_403_FORBIDDEN), + ("staff", "free_text_taxonomy", {"enabled": False}, ["tag1"], status.HTTP_403_FORBIDDEN), + # Can't add invalid/nonexistent tags using a closed taxonomy + (None, "language_taxonomy", {}, ["Invalid"], status.HTTP_401_UNAUTHORIZED), + ("user_1", "language_taxonomy", {}, ["Invalid"], status.HTTP_400_BAD_REQUEST), + ("staff", "language_taxonomy", {}, ["Invalid"], status.HTTP_400_BAD_REQUEST), + ("staff", "taxonomy", {}, ["Invalid"], status.HTTP_400_BAD_REQUEST), ) @ddt.unpack - def test_tag_object(self, user_attr, taxonomy_attr, tag_values, expected_status): + def test_tag_object(self, user_attr, taxonomy_attr, taxonomy_flags, tag_values, expected_status): if user_attr: user = getattr(self, user_attr) self.client.force_authenticate(user=user) taxonomy = getattr(self, taxonomy_attr) + if taxonomy_flags: + for (k, v) in taxonomy_flags.items(): + setattr(taxonomy, k, v) + taxonomy.save() - url = OBJECT_TAGS_UPDATE_URL.format(object_id="abc", taxonomy_id=taxonomy.pk) + object_id = "abc" - response = self.client.put(url, {"tags": tag_values}, format="json") - assert response.status_code == expected_status - if status.is_success(expected_status): - assert len(response.data) == len(tag_values) - assert set(t["value"] for t in response.data) == set(tag_values) - - @ddt.data( - # Can't add invalid tags using a closed taxonomy - (None, "language_taxonomy", ["Invalid"], status.HTTP_401_UNAUTHORIZED), - ("user", "language_taxonomy", ["Invalid"], status.HTTP_400_BAD_REQUEST), - ("staff", "language_taxonomy", ["Invalid"], status.HTTP_400_BAD_REQUEST), - (None, "enabled_taxonomy", ["invalid"], status.HTTP_401_UNAUTHORIZED), - ("user", "enabled_taxonomy", ["invalid"], status.HTTP_400_BAD_REQUEST), - ("staff", "enabled_taxonomy", ["invalid"], status.HTTP_400_BAD_REQUEST), - (None, "multiple_taxonomy", ["invalid"], status.HTTP_401_UNAUTHORIZED), - ("user", "multiple_taxonomy", ["invalid"], status.HTTP_400_BAD_REQUEST), - ("staff", "multiple_taxonomy", ["invalid"], status.HTTP_400_BAD_REQUEST), - # Nobody can edit object tags using a disabled taxonomy. - (None, "disabled_taxonomy", ["invalid"], status.HTTP_401_UNAUTHORIZED), - ("user", "disabled_taxonomy", ["invalid"], status.HTTP_403_FORBIDDEN), - ("staff", "disabled_taxonomy", ["invalid"], status.HTTP_403_FORBIDDEN), - ) - @ddt.unpack - def test_tag_object_invalid(self, user_attr, taxonomy_attr, tag_values, expected_status): - if user_attr: - user = getattr(self, user_attr) - self.client.force_authenticate(user=user) - - taxonomy = getattr(self, taxonomy_attr) - - url = OBJECT_TAGS_UPDATE_URL.format(object_id="abc", taxonomy_id=taxonomy.pk) + url = OBJECT_TAGS_UPDATE_URL.format(object_id=object_id, taxonomy_id=taxonomy.pk) response = self.client.put(url, {"tags": tag_values}, format="json") assert response.status_code == expected_status - assert not status.is_success(expected_status) # No success cases here + if status.is_success(expected_status): + assert [t["value"] for t in response.data[object_id]["taxonomies"][0]["tags"]] == tag_values + # And retrieving the object tags again should return an identical response: + assert response.data == self.client.get(OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id)).data @ddt.data( # Users and staff can clear tags - (None, "enabled_taxonomy", [], status.HTTP_401_UNAUTHORIZED), - ("user", "enabled_taxonomy", [], status.HTTP_200_OK), - ("staff", "enabled_taxonomy", [], status.HTTP_200_OK), - # Users and staff can clear object tags using a allow_multiple=True taxonomy - (None, "multiple_taxonomy", [], status.HTTP_401_UNAUTHORIZED), - ("user", "multiple_taxonomy", [], status.HTTP_200_OK), - ("staff", "multiple_taxonomy", [], status.HTTP_200_OK), + (None, {}, status.HTTP_401_UNAUTHORIZED), + ("user_1", {}, status.HTTP_200_OK), + ("staff", {}, status.HTTP_200_OK), # Nobody can clear tags using a disabled taxonomy - (None, "disabled_taxonomy", [], status.HTTP_401_UNAUTHORIZED), - ("user", "disabled_taxonomy", [], status.HTTP_403_FORBIDDEN), - ("staff", "disabled_taxonomy", [], status.HTTP_403_FORBIDDEN), - (None, "open_taxonomy_disabled", [], status.HTTP_401_UNAUTHORIZED), - ("user", "open_taxonomy_disabled", [], status.HTTP_403_FORBIDDEN), - ("staff", "open_taxonomy_disabled", [], status.HTTP_403_FORBIDDEN), + (None, {"enabled": False}, status.HTTP_401_UNAUTHORIZED), + ("user_1", {"enabled": False}, status.HTTP_403_FORBIDDEN), + ("staff", {"enabled": False}, status.HTTP_403_FORBIDDEN), + # ... and it doesn't matter if it's free text or closed: + ("staff", {"enabled": False, "allow_free_text": False}, status.HTTP_403_FORBIDDEN), ) @ddt.unpack - def test_tag_object_clear(self, user_attr, taxonomy_attr, tag_values, expected_status): - if user_attr: - user = getattr(self, user_attr) - self.client.force_authenticate(user=user) - - taxonomy = getattr(self, taxonomy_attr) + def test_tag_object_clear(self, user_attr, taxonomy_flags, expected_status): + """ + Test that authorized users can *remove* tags using this API + """ + object_id = "abc" - url = OBJECT_TAGS_UPDATE_URL.format(object_id="abc", taxonomy_id=taxonomy.pk) + # First create an object tag: + api.tag_object(object_id=object_id, taxonomy=self.taxonomy, tags=["Fungi"]) - response = self.client.put(url, {"tags": tag_values}, format="json") - assert response.status_code == expected_status - if status.is_success(expected_status): - assert len(response.data) == len(tag_values) - assert set(t["value"] for t in response.data) == set(tag_values) - - @ddt.data( - # Users and staff can add multiple tags using a allow_multiple=True taxonomy - (None, "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_401_UNAUTHORIZED), - ("user", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), - ("staff", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), - (None, "open_taxonomy_enabled", ["tag1", "tag2"], status.HTTP_401_UNAUTHORIZED), - ("user", "open_taxonomy_enabled", ["tag1", "tag2"], status.HTTP_400_BAD_REQUEST), - ("staff", "open_taxonomy_enabled", ["tag1", "tag2"], status.HTTP_400_BAD_REQUEST), - # Users and staff can't add multple tags using a allow_multiple=False taxonomy - (None, "enabled_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_401_UNAUTHORIZED), - ("user", "enabled_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_400_BAD_REQUEST), - ("staff", "enabled_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_400_BAD_REQUEST), - (None, "language_taxonomy", ["Portuguese", "English"], status.HTTP_401_UNAUTHORIZED), - ("user", "language_taxonomy", ["Portuguese", "English"], status.HTTP_400_BAD_REQUEST), - ("staff", "language_taxonomy", ["Portuguese", "English"], status.HTTP_400_BAD_REQUEST), - # Nobody can edit tags using a disabled taxonomy. - (None, "disabled_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_401_UNAUTHORIZED), - ("user", "disabled_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_403_FORBIDDEN), - ("staff", "disabled_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_403_FORBIDDEN), - ) - @ddt.unpack - def test_tag_object_multiple(self, user_attr, taxonomy_attr, tag_values, expected_status): if user_attr: user = getattr(self, user_attr) self.client.force_authenticate(user=user) - taxonomy = getattr(self, taxonomy_attr) + if taxonomy_flags: + for (k, v) in taxonomy_flags.items(): + setattr(self.taxonomy, k, v) + self.taxonomy.save() - url = OBJECT_TAGS_UPDATE_URL.format(object_id="abc", taxonomy_id=taxonomy.pk) + url = OBJECT_TAGS_UPDATE_URL.format(object_id=object_id, taxonomy_id=self.taxonomy.pk) - response = self.client.put(url, {"tags": tag_values}, format="json") + response = self.client.put(url, {"tags": []}, format="json") assert response.status_code == expected_status if status.is_success(expected_status): - assert len(response.data) == len(tag_values) - assert set(t["value"] for t in response.data) == set(tag_values) + # Now there are no tags applied: + assert response.data[object_id]["taxonomies"] == [] + else: + # Make sure the object tags are unchanged: + assert [ot.value for ot in api.get_object_tags(object_id=object_id)] == ["Fungi"] @ddt.data( (None, status.HTTP_401_UNAUTHORIZED), - ("user", status.HTTP_403_FORBIDDEN), + ("user_1", status.HTTP_403_FORBIDDEN), ("staff", status.HTTP_403_FORBIDDEN), ) @ddt.unpack @@ -874,7 +910,7 @@ def test_tag_object_without_permission(self, user_attr, expected_status): user = getattr(self, user_attr) self.client.force_authenticate(user=user) - url = OBJECT_TAGS_UPDATE_URL.format(object_id="view_only", taxonomy_id=self.enabled_taxonomy.pk) + url = OBJECT_TAGS_UPDATE_URL.format(object_id="view_only", taxonomy_id=self.taxonomy.pk) response = self.client.put(url, {"tags": ["Tag 1"]}, format="json") assert response.status_code == expected_status @@ -885,25 +921,82 @@ def test_tag_object_count_limit(self): Checks if the limit of 100 tags per object is enforced """ object_id = "limit_tag_count" - url = OBJECT_TAGS_UPDATE_URL.format(object_id=object_id, taxonomy_id=self.enabled_taxonomy.pk) + dummy_taxonomies = self.create_100_taxonomies() + + url = OBJECT_TAGS_UPDATE_URL.format(object_id=object_id, taxonomy_id=self.taxonomy.pk) self.client.force_authenticate(user=self.staff) response = self.client.put(url, {"tags": ["Tag 1"]}, format="json") # Can't add another tag because the object already has 100 tags assert response.status_code == status.HTTP_400_BAD_REQUEST # The user can edit the tags that are already on the object - for taxonomy in self.dummy_taxonomies: + for taxonomy in dummy_taxonomies: url = OBJECT_TAGS_UPDATE_URL.format(object_id=object_id, taxonomy_id=taxonomy.pk) response = self.client.put(url, {"tags": ["New Tag"]}, format="json") assert response.status_code == status.HTTP_200_OK # Editing tags adding another one will fail - for taxonomy in self.dummy_taxonomies: + for taxonomy in dummy_taxonomies: url = OBJECT_TAGS_UPDATE_URL.format(object_id=object_id, taxonomy_id=taxonomy.pk) response = self.client.put(url, {"tags": ["New Tag 1", "New Tag 2"]}, format="json") assert response.status_code == status.HTTP_400_BAD_REQUEST +class TestObjectTagCountsViewSet(TestTagTaxonomyMixin, APITestCase): + """ + Testing various cases for counting how many tags are applied to several objects. + """ + + def test_get_counts(self): + """ + Test retrieving the counts of tags applied to various content objects. + + This API does NOT bother doing any permission checks as the "# of tags" is not considered sensitive information. + """ + # Course 2 + api.tag_object(object_id="course02-unit01-problem01", taxonomy=self.free_text_taxonomy, tags=["other"]) + # Course 7 Unit 1 + api.tag_object(object_id="course07-unit01-problem01", taxonomy=self.free_text_taxonomy, tags=["a", "b", "c"]) + api.tag_object(object_id="course07-unit01-problem02", taxonomy=self.free_text_taxonomy, tags=["a", "b"]) + # Course 7 Unit 2 + api.tag_object(object_id="course07-unit02-problem01", taxonomy=self.free_text_taxonomy, tags=["b"]) + api.tag_object(object_id="course07-unit02-problem02", taxonomy=self.free_text_taxonomy, tags=["c", "d"]) + api.tag_object(object_id="course07-unit02-problem03", taxonomy=self.free_text_taxonomy, tags=["N", "M", "x"]) + + def check(object_id_pattern: str): + result = self.client.get(OBJECT_TAG_COUNTS_URL.format(object_id_pattern=object_id_pattern)) + assert result.status_code == status.HTTP_200_OK + return result.data + + with self.assertNumQueries(1): + assert check(object_id_pattern="course02-*") == { + "course02-unit01-problem01": 1, + } + with self.assertNumQueries(1): + assert check(object_id_pattern="course07-unit01-*") == { + "course07-unit01-problem01": 3, + "course07-unit01-problem02": 2, + } + with self.assertNumQueries(1): + assert check(object_id_pattern="course07-unit*") == { + "course07-unit01-problem01": 3, + "course07-unit01-problem02": 2, + "course07-unit02-problem01": 1, + "course07-unit02-problem02": 2, + "course07-unit02-problem03": 3, + } + # Can also use a comma to separate explicit object IDs: + with self.assertNumQueries(1): + assert check(object_id_pattern="course07-unit01-problem01") == { + "course07-unit01-problem01": 3, + } + with self.assertNumQueries(1): + assert check(object_id_pattern="course07-unit01-problem01,course07-unit02-problem02") == { + "course07-unit01-problem01": 3, + "course07-unit02-problem02": 2, + } + + class TestTaxonomyTagsView(TestTaxonomyViewMixin): """ Tests the list/create/update/delete tags of taxonomy view