feat: WIP: refactor views to use new get_filtered_tags()

openedx_tagging/core/tagging/models/base.py

@@ -18,6 +18,7 @@
 from openedx_learning.lib.fields import MultiCollationTextField, case_insensitive_char_field, case_sensitive_char_field
 
 from ..data import TagData
+from .utils import ConcatNull
 
 log = logging.getLogger(__name__)
 
@@ -462,9 +463,12 @@ def _get_filtered_tags_deep(
         qs = Tag.annotate_depth(qs)
         # Add the "lineage" field to sort them in order correctly:
         qs = qs.annotate(sort_key=Concat(
-            Coalesce(F("parent__parent__parent__value"), Value("")),
-            Coalesce(F("parent__parent__value"), Value("")),
-            Coalesce(F("parent__value"), Value("")),
+            # For a root tag, we want sort_key="RootValue" and for a depth=1 tag
+            # we want sort_key="RootValue\tValue". The following does that, since
+            # ConcatNull(...) returns NULL if any argument is NULL.
+            Coalesce(ConcatNull(F("parent__parent__parent__value"), Value("\t")), Value("")),
+            Coalesce(ConcatNull(F("parent__parent__value"), Value("\t")), Value("")),
+            Coalesce(ConcatNull(F("parent__value"), Value("\t")), Value("")),
             F("value"),
             output_field=models.CharField(),
         ))

openedx_tagging/core/tagging/models/utils.py

@@ -0,0 +1,24 @@
+"""
+Utilities for tagging and taxonomy models
+"""
+
+from django.db.models.expressions import Func
+
+
+class ConcatNull(Func):
+    """
+    Concatenate two arguments together. Like normal SQL but unlike Django's
+    "Concat", if either argument is NULL, the result will be NULL.
+    """
+
+    function = "CONCAT"
+
+    def as_sqlite(self, compiler, connection, **extra_context):
+        """ SQLite doesn't have CONCAT() but has a concatenation operator """
+        return super().as_sql(
+            compiler,
+            connection,
+            template="%(expressions)s",
+            arg_joiner=" || ",
+            **extra_context,
+        )

openedx_tagging/core/tagging/rest_api/v1/serializers.py

@@ -6,7 +6,7 @@
 from rest_framework.reverse import reverse
 
 from openedx_tagging.core.tagging.data import TagData
-from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy
+from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy
 
 
 class TaxonomyListQueryParamsSerializer(serializers.Serializer):  # pylint: disable=abstract-method
@@ -126,104 +126,3 @@ def get_sub_tags_url(self, obj: TagData):
             )
             return request.build_absolute_uri(url)
         return None
-
-
-class TagsSerializer(serializers.ModelSerializer):
-    """
-    Serializer for Tags
-
-    Adds a link to get the sub tags
-    """
-
-    sub_tags_link = serializers.SerializerMethodField()
-    children_count = serializers.SerializerMethodField()
-
-    class Meta:
-        model = Tag
-        fields = (
-            "id",
-            "value",
-            "taxonomy_id",
-            "parent_id",
-            "sub_tags_link",
-            "children_count",
-        )
-
-    def get_sub_tags_link(self, obj):
-        """
-        Returns URL for the list of child tags of the current tag.
-        """
-        if obj.children.count():
-            query_params = f"?parent_tag_id={obj.id}"
-            url = (
-                reverse("oel_tagging:taxonomy-tags", args=[str(obj.taxonomy_id)])
-                + query_params
-            )
-            request = self.context.get("request")
-            return request.build_absolute_uri(url)
-        return None
-
-    def get_children_count(self, obj):
-        """
-        Returns the number of child tags of the given tag.
-        """
-        return obj.children.count()
-
-
-class TagsWithSubTagsSerializer(serializers.ModelSerializer):
-    """
-    Serializer for Tags.
-
-    Represents a tree with a list of sub tags
-    """
-
-    sub_tags = serializers.SerializerMethodField()
-    children_count = serializers.SerializerMethodField()
-
-    class Meta:
-        model = Tag
-        fields = (
-            "id",
-            "value",
-            "taxonomy_id",
-            "sub_tags",
-            "children_count",
-        )
-
-    def get_sub_tags(self, obj):
-        """
-        Returns a serialized list of child tags for the given tag.
-        """
-        serializer = TagsWithSubTagsSerializer(
-            obj.children.all().order_by("value", "id"),
-            many=True,
-            read_only=True,
-        )
-        return serializer.data
-
-    def get_children_count(self, obj):
-        """
-        Returns the number of child tags of the given tag.
-        """
-        return obj.children.count()
-
-
-class TagsForSearchSerializer(TagsWithSubTagsSerializer):
-    """
-    Serializer for Tags
-
-    Used to filter sub tags of a given tag
-    """
-
-    def get_sub_tags(self, obj):
-        """
-        Returns a serialized list of child tags for the given tag.
-        """
-        serializer = TagsWithSubTagsSerializer(obj.sub_tags, many=True, read_only=True)
-        return serializer.data
-
-    def get_children_count(self, obj):
-        """
-        Returns the number of child tags of the given tag.
-        """
-        return len(obj.sub_tags)

openedx_tagging/core/tagging/rest_api/v1/views.py

@@ -14,32 +14,20 @@
 
 from openedx_tagging.core.tagging.models.base import Tag
 
-from ...api import (
-    create_taxonomy,
-    get_children_tags,
-    get_object_tags,
-    get_root_tags,
-    get_taxonomies,
-    get_taxonomy,
-    search_tags,
-    tag_object,
-)
+from ...api import create_taxonomy, get_object_tags, get_taxonomies, get_taxonomy, tag_object
+from ...data import TagData
 from ...import_export.api import export_tags
 from ...import_export.parsers import ParserFormat
 from ...models import Taxonomy
-from ...data import TagData
 from ...rules import ObjectTagPermissionItem
-from ..paginators import SEARCH_TAGS_THRESHOLD, TAGS_THRESHOLD, DisabledTagsPagination, TagsPagination
+from ..paginators import TAGS_THRESHOLD, DisabledTagsPagination, TagsPagination
 from .permissions import ObjectTagObjectPermissions, TagListPermissions, TaxonomyObjectPermissions
 from .serializers import (
     ObjectTagListQueryParamsSerializer,
     ObjectTagSerializer,
     ObjectTagUpdateBodySerializer,
     ObjectTagUpdateQueryParamsSerializer,
     TagDataSerializer,
-    TagsForSearchSerializer,
-    TagsSerializer,
-    TagsWithSubTagsSerializer,
     TaxonomyExportQueryParamsSerializer,
     TaxonomyListQueryParamsSerializer,
     TaxonomySerializer,
@@ -414,17 +402,22 @@ class TaxonomyTagsView(ListAPIView):
     hierachy will be returned. Otherwise, several levels will be returned, in
     tree order, up to the maximum supported depth. Additional levels/depth can
     be retrieved by using ?parent_tag_value to load more data.
+    
+    Note: If the taxonomy is particularly large (> 1,000 tags), ?root_only is
+    automatically set true by default and cannot be disabled. This way, users
+    can more easily select which tags they want to expand in the tree, and load
+    just that subset of the tree as needed. This may be changed in the future.
 
     **List Query Parameters**
-        * pk (required) - The pk of the taxonomy to retrieve tags.
-        * parent_tag_value (optional) - Id of the tag to retrieve children tags.
+        * id (required) - The ID of the taxonomy to retrieve tags.
+        * parent_tag (optional) - Retrieve children of the tag with this value.
         * root_only (optional) - If specified, only root tags are returned.
         * page (optional) - Page number (default: 1)
         * page_size (optional) - Number of items per page (default: 10)
 
     **List Example Requests**
-        GET api/tagging/v1/taxonomy/:pk/tags                                        - Get tags of taxonomy
-        GET api/tagging/v1/taxonomy/:pk/tags?parent_tag_id=30                       - Get children tags of tag
+        GET api/tagging/v1/taxonomy/:id/tags                                        - Get tags of taxonomy
+        GET api/tagging/v1/taxonomy/:id/tags?parent_tag=Physics                     - Get child tags of tag
 
     **List Query Returns**
         * 200 - Success
@@ -467,13 +460,17 @@ def get_queryset(self) -> models.QuerySet[TagData]:
 
         if parent_tag_value:
             # Fetching tags below a certain parent is always paginated and only returns the direct children
-            self.pagination_enabled = True
             depth = 1
             if root_only:
-                raise ValidationError("?root_only and ?parent_tag_value cannot be used together")
+                raise ValidationError("?root_only and ?parent_tag cannot be used together")
         else:
-            if root_only or taxonomy.tag_set.count() > TAGS_THRESHOLD:
-                depth = 1  # Load only the root tags for now
+            if root_only:
+                depth = 1  # User Explicitly requested to load only the root tags for now
+            elif search_term:
+                depth = None  # For search, default to maximum depth but use normal pagination
+            elif taxonomy.tag_set.count() > TAGS_THRESHOLD:
+                # This is a very large taxonomy. Only load the root tags at first, so users can choose what to load.
+                depth = 1
             else:
                 # We can load and display all the tags in the taxonomy at once:
                 self.pagination_class = DisabledTagsPagination

tests/openedx_tagging/core/tagging/import_export/test_template.py

@@ -12,8 +12,8 @@
 from openedx_tagging.core.tagging.import_export import ParserFormat
 from openedx_tagging.core.tagging.import_export import api as import_api
 
-from .mixins import TestImportExportMixin
 from ..utils import pretty_format_tags
+from .mixins import TestImportExportMixin
 
 
 @ddt.ddt

tests/openedx_tagging/core/tagging/test_api.py

@@ -644,22 +644,6 @@ def test_autocomplete_tags(self, search: str, expected_values: list[str], expect
                 _value=value,
             ).save()
 
-        # Test for open taxonomy
-        # self._validate_autocomplete_tags(
-        #     open_taxonomy,
-        #     search,
-        #     expected_values,
-        #     [None] * len(expected_ids),
-        # )
-
-        # # Test for closed taxonomy
-        # self._validate_autocomplete_tags(
-        #     closed_taxonomy,
-        #     search,
-        #     expected_values,
-        #     expected_ids,
-        # )
-
     @ddt.data(
         ("ChA", [
             "Archaea (used: 1, children: 3)",
@@ -756,37 +740,3 @@ def _get_tag_ids(self, tags) -> list[int]:
         Get tag ids from tagging_api.autocomplete_tags() result
         """
         return [tag.get("tag_id") for tag in tags]
-
-    def _validate_autocomplete_tags(
-        self,
-        taxonomy: Taxonomy,
-        search: str,
-        expected: list[str],
-    ) -> None:
-        """
-        Validate autocomplete tags
-        """
-
-        # Normal search
-        result = tagging_api.search_tags(taxonomy, search)
-
-        assert pretty_format_tags(result, parent=False) == expected
-
-        # Create ObjectTag to simulate the content tagging
-        first_value = next(t for t in result if search.lower() in t["value"].lower())
-        tag_model = None
-        if not taxonomy.allow_free_text:
-            tag_model = get_tag(first_value)
-
-        object_id = 'new_object_id'
-        ObjectTag(
-            object_id=object_id,
-            taxonomy=taxonomy,
-            tag=tag_model,
-            _value=first_value,
-        ).save()
-
-        # Search with object
-        result = tagging_api.search_tags(taxonomy, search, object_id)
-        assert self._get_tag_values(result) == expected_values[1:]
-        assert self._get_tag_ids(result) == expected_ids[1:]

tests/openedx_tagging/core/tagging/test_models.py

@@ -13,6 +13,7 @@
 
 from openedx_tagging.core.tagging import api
 from openedx_tagging.core.tagging.models import LanguageTaxonomy, ObjectTag, Tag, Taxonomy
+
 from .utils import pretty_format_tags
 
 
@@ -443,6 +444,33 @@ def test_usage_count(self) -> None:
             "Bacteria (None) (used: 3, children: 2)",
         ]
 
+    def test_pathological_tree_sort(self) -> None:
+        """
+        Check for bugs in how tree sorting happens, if the tag names are very
+        similar.
+        """
+        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)
+        result = pretty_format_tags(taxonomy.get_filtered_tags())
+        assert result == [
+            "1 (None) (used: 0, children: 4)",
+            "  1 A (1) (used: 0, children: 0)",
+            "  11 (1) (used: 0, children: 0)",
+            "  11111 (1) (used: 0, children: 1)",
+            "    1111-grandchild (11111) (used: 0, children: 0)",
+            "  2 (1) (used: 0, children: 0)",
+            "111 (None) (used: 0, children: 2)",
+            "  11111111 (111) (used: 0, children: 0)",
+            "  123 (111) (used: 0, children: 0)",
+        ]
 
 class TestFilteredTagsFreeTextTaxonomy(TestCase):
     """