From 0473c0ea2bc78e899c10fb6f7a84dcdc5dd9bec6 Mon Sep 17 00:00:00 2001 From: tbain Date: Wed, 18 Mar 2026 11:21:49 -0700 Subject: [PATCH 01/17] feat: #253 new branch to reduce noise; adding BE logic to implement counting logic with unit tests --- src/openedx_tagging/models/base.py | 65 +++++--- tests/openedx_tagging/test_models.py | 227 ++++++++++++++++++++++++++- 2 files changed, 271 insertions(+), 21 deletions(-) diff --git a/src/openedx_tagging/models/base.py b/src/openedx_tagging/models/base.py index 668ba73e3..8394b8ed8 100644 --- a/src/openedx_tagging/models/base.py +++ b/src/openedx_tagging/models/base.py @@ -9,8 +9,8 @@ from django.core.exceptions import ValidationError from django.db import models -from django.db.models import F, Q, Value -from django.db.models.functions import Concat, Lower +from django.db.models import Count, F, IntegerField, OuterRef, Q, Subquery, Value +from django.db.models.functions import Coalesce, Concat, Lower from django.utils.functional import cached_property from django.utils.module_loading import import_string from django.utils.translation import gettext_lazy as _ @@ -25,6 +25,8 @@ # Maximum depth allowed for a hierarchical taxonomy's tree of tags. +# Note: if this changes please check logic in this file for notes +# about necessary changes TAXONOMY_MAX_DEPTH = 3 # Ancestry of a given tag; the Tag.value fields of a given tag and its parents, starting from the root. @@ -501,16 +503,7 @@ def _get_filtered_tags_one_level( qs = qs.values("value", "child_count", "descendant_count", "depth", "parent_value", "external_id", "_id") qs = qs.order_by("value") if include_counts: - # We need to include the count of how many times this tag is used to tag objects. - # You'd think we could just use: - # qs = qs.annotate(usage_count=models.Count("objecttag__pk")) - # but that adds another join which starts creating a cross product and the children and usage_count become - # intertwined and multiplied with each other. So we use a subquery. - obj_tags = ObjectTag.objects.filter(tag_id=models.OuterRef("pk")).order_by().annotate( - # We need to use Func() to get Count() without GROUP BY - see https://stackoverflow.com/a/69031027 - count=models.Func(F('id'), function='Count') - ) - qs = qs.annotate(usage_count=models.Subquery(obj_tags.values('count'))) + qs = self.add_counts_query(qs) return qs # type: ignore[return-value] def _get_filtered_tags_deep( @@ -593,14 +586,50 @@ def _get_filtered_tags_deep( qs = qs.values("value", "child_count", "descendant_count", "depth", "parent_value", "external_id", "_id") qs = qs.order_by("sort_key") if include_counts: - # Including the counts is a bit tricky; see the comment above in _get_filtered_tags_one_level() - obj_tags = ObjectTag.objects.filter(tag_id=models.OuterRef("pk")).order_by().annotate( - # We need to use Func() to get Count() without GROUP BY - see https://stackoverflow.com/a/69031027 - count=models.Func(F('id'), function='Count') - ) - qs = qs.annotate(usage_count=models.Subquery(obj_tags.values('count'))) + qs = self.add_counts_query(qs) + return qs # type: ignore[return-value] + def add_counts_query(self, qs: models.QuerySet ): + # Adds a subquery to the passed-in queryset that returns the number + # of times a tag has been used. + # + # Note: The count is not a simple count, we need to do a 'roll up' + # where we count the number of times a tag is directly used and applied, + # but then that also needs to add a "1" count to the lineage tags + # (parent, grandparent, etc.), but de-duplicate counts for any children + # so that if we have "2" child tags, it only counts towards "1" for the + # parent. + # This query gets the raw counts for each tag usage, gets the distinct + # usages (so de-duplicates counts) by actual application to an "Object" + # (library, course, course module, course section, etc.), which creates + # a count per tag, annotated to that particular tag from the passed-in + # queryset. + # + # Note: This only works with a tag lineage depth of "3" (the now + # current value of TAXONOMY_MAX_DEPTH), inclusive of 0, so 0...3 + # if we change TAXONOMY_MAX_DEPTH this code will need to be updated. + + assert TAXONOMY_MAX_DEPTH == 3 # If we change TAXONOMY_MAX_DEPTH we need to change this query code + usage_count_qs = ObjectTag.objects.filter( + Q(tag_id=OuterRef('pk')) | + Q(tag__parent_id=OuterRef('pk')) | + Q(tag__parent__parent_id=OuterRef('pk')) | + Q(tag__parent__parent__parent_id=OuterRef('pk')) + ).values('object_id').distinct().annotate( + intermediate_grouping=Value(1, output_field=IntegerField()) + ).values('intermediate_grouping').annotate( + total_usage=Count('object_id', distinct=True) + ).values('total_usage') + + qs = qs.annotate( + usage_count=Coalesce( + Subquery(usage_count_qs, output_field=IntegerField()), + 0 # Coalesce ensures we return 0 instead of None if there are no usages + ) + ) + return qs + def add_tag( self, tag_value: str, diff --git a/tests/openedx_tagging/test_models.py b/tests/openedx_tagging/test_models.py index 67ae68e2f..a0ef9cce4 100644 --- a/tests/openedx_tagging/test_models.py +++ b/tests/openedx_tagging/test_models.py @@ -50,6 +50,7 @@ def setUp(self): self.chordata = get_tag("Chordata") self.mammalia = get_tag("Mammalia") self.animalia = get_tag("Animalia") + self.eukaryota = get_tag("Eukaryota") 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()( @@ -543,7 +544,9 @@ def test_get_external_id(self) -> None: def test_usage_count(self) -> None: """ - Test that the usage count in the results is right + Test that the usage count in the results is right for a basic case; + many objects tagged seperately should return a simple usage count that + reflects lineage de-duplication (or lack thereof, in this case) """ api.tag_object(object_id="obj01", taxonomy=self.taxonomy, tags=["Bacteria"]) api.tag_object(object_id="obj02", taxonomy=self.taxonomy, tags=["Bacteria"]) @@ -552,7 +555,7 @@ def test_usage_count(self) -> None: # Now the API should reflect these usage counts: result = pretty_format_tags(self.taxonomy.get_filtered_tags(search_term="bacteria", include_counts=True)) assert result == [ - "Bacteria (None) (used: 3, children: 2)", + "Bacteria (None) (used: 4, children: 2)", " Archaebacteria (Bacteria) (used: 0, children: 0)", " Eubacteria (Bacteria) (used: 1, children: 0)", ] @@ -561,9 +564,227 @@ def test_usage_count(self) -> None: self.taxonomy.get_filtered_tags(search_term="bacteria", include_counts=True, depth=1) ) assert result1 == [ - "Bacteria (None) (used: 3, children: 2)", + "Bacteria (None) (used: 4, children: 2)", ] + def test_usage_count_lineage_count_across_same_course(self) -> None: + """ + Test that the usage count is correct and parent counts are included based on + child tags being added to an object. However, we de-duplicate and only count + 1 parent tag towards a course even if 2 children are applied to that course + """ + api.tag_object(object_id="obj01", taxonomy=self.taxonomy, tags=["Bacteria"]) + api.tag_object(object_id="obj01", taxonomy=self.taxonomy, tags=["Archaebacteria"]) + api.tag_object(object_id="obj02", taxonomy=self.taxonomy, tags=["Archaebacteria"]) + api.tag_object(object_id="obj01", taxonomy=self.taxonomy, tags=["Eubacteria"]) + # Now the API should reflect these usage counts: + result = pretty_format_tags(self.taxonomy.get_filtered_tags(search_term="bacteria", include_counts=True)) + assert result == [ + "Bacteria (None) (used: 2, children: 2)", + " Archaebacteria (Bacteria) (used: 1, children: 0)", + " Eubacteria (Bacteria) (used: 1, children: 0)", + ] + # Same with depth=1, which uses a different query internally: + result1 = pretty_format_tags( + self.taxonomy.get_filtered_tags(search_term="bacteria", include_counts=True, depth=1) + ) + assert result1 == [ + "Bacteria (None) (used: 2, children: 2)", + ] + + def test_usage_count_rolls_up_to_ancestors_deep(self) -> None: + """ + AI/Claude4.6 generated via IntelliJ IDEA AI Assistant + When a child tag (depth 3) is applied to an object, it should + roll up the count to all its ancestors when using _get_filtered_tags_deep. + The child tag and each of its ancestors should have usage_count=1. + """ + api.tag_object("obj:1", self.taxonomy, [self.mammalia.value]) + result = pretty_format_tags(self.taxonomy.get_filtered_tags(include_counts=True)) + assert result == [ + "Archaea (None) (used: 0, children: 3)", + " DPANN (Archaea) (used: 0, children: 0)", + " Euryarchaeida (Archaea) (used: 0, children: 0)", + " Proteoarchaeota (Archaea) (used: 0, children: 0)", + "Bacteria (None) (used: 0, children: 2)", + " Archaebacteria (Bacteria) (used: 0, children: 0)", + " Eubacteria (Bacteria) (used: 0, children: 0)", + "Eukaryota (None) (used: 1, children: 5 + 8)", + " Animalia (Eukaryota) (used: 1, children: 7 + 1)", + " Arthropoda (Animalia) (used: 0, children: 0)", + " Chordata (Animalia) (used: 1, children: 1)", + " Cnidaria (Animalia) (used: 0, children: 0)", + " Ctenophora (Animalia) (used: 0, children: 0)", + " Gastrotrich (Animalia) (used: 0, children: 0)", + " Placozoa (Animalia) (used: 0, children: 0)", + " Porifera (Animalia) (used: 0, children: 0)", + " Fungi (Eukaryota) (used: 0, children: 0)", + " Monera (Eukaryota) (used: 0, children: 0)", + " Plantae (Eukaryota) (used: 0, children: 0)", + " Protista (Eukaryota) (used: 0, children: 0)", + ] + + def test_usage_count_multiple_objects_same_tag_deep(self) -> None: + """ + AI/Claude4.6 generated via IntelliJ IDEA AI Assistant + When two distinct objects (e.g. seperate courses, modules, etc.) are tagged + with the same child tag, it should count 2 for that tag (and roll up 2 + to ancestors). Each distinct object should contribute exactly 1 to the count. + """ + api.tag_object("obj:1", self.taxonomy, [self.chordata.value]) + api.tag_object("obj:2", self.taxonomy, [self.chordata.value]) + result = pretty_format_tags( + self.taxonomy.get_filtered_tags(search_term="chordata", include_counts=True) + ) + assert result == [ + "Eukaryota (None) (used: 2, children: 1 + 1)", + " Animalia (Eukaryota) (used: 2, children: 1)", + " Chordata (Animalia) (used: 2, children: 0)", + ] + + def test_usage_count_sibling_tags_same_object_deduplication_deep(self) -> None: + """ + AI/Claude4.6 generated via IntelliJ IDEA AI Assistant + When one object is tagged with two sibling tags (both children of the same + parent), the parent's usage_count should be 1, not 2. It should de-duplicate. + """ + self.taxonomy.allow_multiple = True + self.taxonomy.save() + # Eubacteria and Archaebacteria are both children of Bacteria + api.tag_object("obj:1", self.taxonomy, [self.eubacteria.value, self.archaebacteria.value]) + result = pretty_format_tags( + self.taxonomy.get_filtered_tags(search_term="bacteria", include_counts=True) + ) + assert result == [ + "Bacteria (None) (used: 1, children: 2)", + " Archaebacteria (Bacteria) (used: 1, children: 0)", + " Eubacteria (Bacteria) (used: 1, children: 0)", + ] + + def test_usage_count_sibling_tags_different_objects_deep(self) -> None: + """ + AI/Claude4.6 generated via IntelliJ IDEA AI Assistant + When two different objects are each tagged with a different sibling tag, + the parent's usage_count should be 2, not 1. + """ + api.tag_object("obj:1", self.taxonomy, [self.eubacteria.value]) + api.tag_object("obj:2", self.taxonomy, [self.archaebacteria.value]) + result = pretty_format_tags( + self.taxonomy.get_filtered_tags(search_term="bacteria", include_counts=True) + ) + assert result == [ + "Bacteria (None) (used: 2, children: 2)", + " Archaebacteria (Bacteria) (used: 1, children: 0)", + " Eubacteria (Bacteria) (used: 1, children: 0)", + ] + + def test_usage_count_one_level_root_tags(self) -> None: + """ + AI/Claude4.6 generated via IntelliJ IDEA AI Assistant + _get_filtered_tags_one_level (depth=1) with include_counts=True should + reflect the rolled-up usage count, not just direct usage. + Tagging an object with a child tag should increment the root tag's count. + """ + api.tag_object("obj:1", self.taxonomy, [self.eubacteria.value]) # child of Bacteria + result = pretty_format_tags( + self.taxonomy.get_filtered_tags(depth=1, include_counts=True) + ) + assert result == [ + "Archaea (None) (used: 0, children: 3)", + "Bacteria (None) (used: 1, children: 2)", + "Eukaryota (None) (used: 0, children: 5 + 8)", + ] + + def test_usage_count_one_level_child_tags(self) -> None: + """ + AI/Claude4.6 generated via IntelliJ IDEA AI Assistant + When listing children of a tag (depth=1, parent_tag_value=...), the + usage_count of each child should only reflect the objects tagged with + that child or any of its descendants. + """ + api.tag_object("obj:1", self.taxonomy, [self.mammalia.value]) # grandchild of Animalia via Chordata + api.tag_object("obj:2", self.taxonomy, [self.chordata.value]) # direct child of Animalia + result = pretty_format_tags( + self.taxonomy.get_filtered_tags(depth=1, parent_tag_value="Animalia", include_counts=True) + ) + assert result == [ + " Arthropoda (Animalia) (used: 0, children: 0)", + " Chordata (Animalia) (used: 2, children: 1)", + " Cnidaria (Animalia) (used: 0, children: 0)", + " Ctenophora (Animalia) (used: 0, children: 0)", + " Gastrotrich (Animalia) (used: 0, children: 0)", + " Placozoa (Animalia) (used: 0, children: 0)", + " Porifera (Animalia) (used: 0, children: 0)", + ] + + def test_usage_count_three_levels_deep_rollup(self) -> None: + """ + AI/Claude4.6 generated via IntelliJ IDEA AI Assistant + Tagging an object with a depth-3 tag (Chordata) should roll up + to grandparent (Animalia) and great-grandparent (Eukaryota), + verifying the full 3-level lineage query in add_counts_query. + """ + api.tag_object("obj:1", self.taxonomy, [self.animalia.value]) + api.tag_object("obj:1", self.taxonomy, [self.chordata.value]) + result = pretty_format_tags( + self.taxonomy.get_filtered_tags(search_term="chordata", include_counts=True) + ) + assert result == [ + "Eukaryota (None) (used: 1, children: 1 + 1)", + " Animalia (Eukaryota) (used: 1, children: 1)", + " Chordata (Animalia) (used: 1, children: 0)", + ] + + def test_usage_count_returns_zero_not_none_deep(self) -> None: + """ + AI/Claude4.6 generated via IntelliJ IDEA AI Assistant + When no object has been tagged with a tag or any of its + descendants, usage_count must be 0 (integer), not None. + """ + result = pretty_format_tags(self.taxonomy.get_filtered_tags(include_counts=True)) + assert result == [ + "Archaea (None) (used: 0, children: 3)", + " DPANN (Archaea) (used: 0, children: 0)", + " Euryarchaeida (Archaea) (used: 0, children: 0)", + " Proteoarchaeota (Archaea) (used: 0, children: 0)", + "Bacteria (None) (used: 0, children: 2)", + " Archaebacteria (Bacteria) (used: 0, children: 0)", + " Eubacteria (Bacteria) (used: 0, children: 0)", + "Eukaryota (None) (used: 0, children: 5 + 8)", + " Animalia (Eukaryota) (used: 0, children: 7 + 1)", + " Arthropoda (Animalia) (used: 0, children: 0)", + " Chordata (Animalia) (used: 0, children: 1)", + " Cnidaria (Animalia) (used: 0, children: 0)", + " Ctenophora (Animalia) (used: 0, children: 0)", + " Gastrotrich (Animalia) (used: 0, children: 0)", + " Placozoa (Animalia) (used: 0, children: 0)", + " Porifera (Animalia) (used: 0, children: 0)", + " Fungi (Eukaryota) (used: 0, children: 0)", + " Monera (Eukaryota) (used: 0, children: 0)", + " Plantae (Eukaryota) (used: 0, children: 0)", + " Protista (Eukaryota) (used: 0, children: 0)", + ] + + def test_usage_count_with_search_term_deep(self) -> None: + """ + AI/Claude4.6 generated via IntelliJ IDEA AI Assistant + When using get_filtered_tags() with both a search_term and + include_counts=True, the usage_count returned should still + reflect the true count for each matching tag, not be affected + by the search filter. + """ + api.tag_object("obj:1", self.taxonomy, [self.eubacteria.value]) + api.tag_object("obj:2", self.taxonomy, [self.archaebacteria.value]) + result = pretty_format_tags( + self.taxonomy.get_filtered_tags(search_term="bacteria", include_counts=True) + ) + assert result == [ + "Bacteria (None) (used: 2, children: 2)", + " Archaebacteria (Bacteria) (used: 1, children: 0)", + " Eubacteria (Bacteria) (used: 1, children: 0)", + ] + + def test_tree_sort(self) -> None: """ Verify that taxonomies can be sorted correctly in tree orer (case insensitive). From 2c0b959b99b4264a50788131d98191368a1b7b29 Mon Sep 17 00:00:00 2001 From: tbain Date: Mon, 23 Mar 2026 15:32:29 -0700 Subject: [PATCH 02/17] feat: #253 Fixing API tests with regard to count logic changes --- tests/openedx_tagging/test_api.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/openedx_tagging/test_api.py b/tests/openedx_tagging/test_api.py index aaa372b4b..7797c7f05 100644 --- a/tests/openedx_tagging/test_api.py +++ b/tests/openedx_tagging/test_api.py @@ -753,17 +753,17 @@ def get_object_tags(): "Archaea (used: 1, children: 2)", " Euryarchaeida (used: 0, children: 0)", " Proteoarchaeota (used: 0, children: 0)", - "Bacteria (used: 0, children: 1)", # does not contain "cha" but a child does + "Bacteria (used: 1, children: 1)", # does not contain "cha" but a child does " Archaebacteria (used: 1, children: 0)", ]), ("ar", [ "Archaea (used: 1, children: 2)", " Euryarchaeida (used: 0, children: 0)", " Proteoarchaeota (used: 0, children: 0)", - "Bacteria (used: 0, children: 1)", # does not contain "ar" but a child does + "Bacteria (used: 1, children: 1)", # does not contain "ar" but a child does " Archaebacteria (used: 1, children: 0)", - "Eukaryota (used: 0, children: 1 + 2)", - " Animalia (used: 1, children: 2)", # does not contain "ar" but a child does + "Eukaryota (used: 6, children: 1 + 2)", + " Animalia (used: 4, children: 2)", # does not contain "ar" but a child does " Arthropoda (used: 1, children: 0)", " Cnidaria (used: 0, children: 0)", ]), @@ -771,9 +771,9 @@ def get_object_tags(): "Archaea (used: 1, children: 2)", " Euryarchaeida (used: 0, children: 0)", " Proteoarchaeota (used: 0, children: 0)", - "Bacteria (used: 0, children: 1)", # does not contain "ae" but a child does + "Bacteria (used: 1, children: 1)", # does not contain "ae" but a child does " Archaebacteria (used: 1, children: 0)", - "Eukaryota (used: 0, children: 1)", # does not contain "ae" but a child does + "Eukaryota (used: 6, children: 1)", # does not contain "ae" but a child does " Plantae (used: 1, children: 0)", ]), ("a", [ @@ -781,11 +781,11 @@ def get_object_tags(): " DPANN (used: 0, children: 0)", " Euryarchaeida (used: 0, children: 0)", " Proteoarchaeota (used: 0, children: 0)", - "Bacteria (used: 0, children: 2)", + "Bacteria (used: 1, children: 2)", " Archaebacteria (used: 1, children: 0)", " Eubacteria (used: 0, children: 0)", - "Eukaryota (used: 0, children: 4 + 7)", - " Animalia (used: 1, children: 7)", + "Eukaryota (used: 6, children: 4 + 7)", + " Animalia (used: 4, children: 7)", " Arthropoda (used: 1, children: 0)", " Chordata (used: 0, children: 0)", # <<< Chordata has a matching child but we only support searching " Cnidaria (used: 0, children: 0)", # 3 levels deep at once for now. From b01964b4a75e9d6c18bc290d1805d029dce5f991 Mon Sep 17 00:00:00 2001 From: tbain Date: Thu, 26 Mar 2026 10:07:42 -0700 Subject: [PATCH 03/17] feat: #253 Resolving merge conflict with upstream main branch --- src/openedx_tagging/models/base.py | 37 +++++++++++++++++++--------- tests/openedx_tagging/test_api.py | 7 +++--- tests/openedx_tagging/test_models.py | 2 ++ 3 files changed, 32 insertions(+), 14 deletions(-) diff --git a/src/openedx_tagging/models/base.py b/src/openedx_tagging/models/base.py index 2870b15ff..6167f79e9 100644 --- a/src/openedx_tagging/models/base.py +++ b/src/openedx_tagging/models/base.py @@ -5,13 +5,15 @@ from __future__ import annotations import logging +import operator import re +from functools import reduce from typing import List, Self from django.core.exceptions import ValidationError from django.db import models -from django.db.models import Count, F, IntegerField, OuterRef, Q, Subquery, Value -from django.db.models.functions import Coalesce, Concat, Lower +from django.db.models import Count, F, IntegerField, Q, Subquery, Value +from django.db.models.functions import Coalesce, 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 _ @@ -654,17 +656,30 @@ def add_counts_query(self, qs: models.QuerySet ): # (library, course, course module, course section, etc.), which creates # a count per tag, annotated to that particular tag from the passed-in # queryset. - # - # Note: This only works with a tag lineage depth of "3" (the now - # current value of TAXONOMY_MAX_DEPTH), inclusive of 0, so 0...3 - # if we change TAXONOMY_MAX_DEPTH this code will need to be updated. - assert TAXONOMY_MAX_DEPTH == 3 # If we change TAXONOMY_MAX_DEPTH we need to change this query code + # Since Depth may change depending on the value of TAXONOMY_MAX_DEPTH, dynamically + # build a list of lineage paths to be used in the query, so we're not hard coding to + # a certain number of levels. This will build an array containing something like: + # ['tag_id', 'tag__parent_id', 'tag__parent__parent_id', 'tag__parent__parent__parent_id', ...] + lineage_paths = [f"tag{'__parent' * i}_id" for i in range(0, TAXONOMY_MAX_DEPTH+1)] + + # Combine the above-built lineage with a Q query against the OuterRef("pk"), + lineage_query_list = [Q(**{path: models.OuterRef("pk")}) for path in lineage_paths] + usage_count_qs = ObjectTag.objects.filter( - Q(tag_id=OuterRef('pk')) | - Q(tag__parent_id=OuterRef('pk')) | - Q(tag__parent__parent_id=OuterRef('pk')) | - Q(tag__parent__parent__parent_id=OuterRef('pk')) + # Combine the logic built above with an or operator to flesh out a + # lineage query of the form: + # ``` + # Q(tag_id=OuterRef('pk')) | + # Q(tag__parent_id=OuterRef('pk')) | + # Q(tag__parent__parent_id=OuterRef('pk')) | + # ... + # ``` + # Previously the above was hard coded and needed to be changed with every + # change in TAXONOMY_MAX_DEPTH, now it is dynamic to reduce maintenace + # (Thanks Google for helping me build this) + + reduce(operator.or_, lineage_query_list) ).values('object_id').distinct().annotate( intermediate_grouping=Value(1, output_field=IntegerField()) ).values('intermediate_grouping').annotate( diff --git a/tests/openedx_tagging/test_api.py b/tests/openedx_tagging/test_api.py index 578aaddd8..3f0d08e69 100644 --- a/tests/openedx_tagging/test_api.py +++ b/tests/openedx_tagging/test_api.py @@ -786,10 +786,11 @@ def get_object_tags(): "Bacteria (used: 1, children: 2)", " Archaebacteria (used: 1, children: 0)", " Eubacteria (used: 0, children: 0)", - "Eukaryota (used: 6, children: 4 + 7)", - " Animalia (used: 4, children: 7)", + "Eukaryota (used: 6, children: 4 + 8)", + " Animalia (used: 4, children: 7 + 1)", " Arthropoda (used: 1, children: 0)", - " Chordata (used: 0, children: 0)", # <<< Chordata has a matching child but we only support searching + " Chordata (used: 0, children: 1)", # <<< Chordata has a matching child but we only support searching + " Mammalia (used: 0, children: 0)", " Cnidaria (used: 0, children: 0)", # 3 levels deep at once for now. " Ctenophora (used: 0, children: 0)", " Gastrotrich (used: 1, children: 0)", diff --git a/tests/openedx_tagging/test_models.py b/tests/openedx_tagging/test_models.py index f27f317cf..c9df5255c 100644 --- a/tests/openedx_tagging/test_models.py +++ b/tests/openedx_tagging/test_models.py @@ -615,6 +615,7 @@ def test_usage_count_rolls_up_to_ancestors_deep(self) -> None: " Animalia (Eukaryota) (used: 1, children: 7 + 1)", " Arthropoda (Animalia) (used: 0, children: 0)", " Chordata (Animalia) (used: 1, children: 1)", + " Mammalia (Chordata) (used: 1, children: 0)", " Cnidaria (Animalia) (used: 0, children: 0)", " Ctenophora (Animalia) (used: 0, children: 0)", " Gastrotrich (Animalia) (used: 0, children: 0)", @@ -756,6 +757,7 @@ def test_usage_count_returns_zero_not_none_deep(self) -> None: " Animalia (Eukaryota) (used: 0, children: 7 + 1)", " Arthropoda (Animalia) (used: 0, children: 0)", " Chordata (Animalia) (used: 0, children: 1)", + " Mammalia (Chordata) (used: 0, children: 0)", " Cnidaria (Animalia) (used: 0, children: 0)", " Ctenophora (Animalia) (used: 0, children: 0)", " Gastrotrich (Animalia) (used: 0, children: 0)", From a23afe80967109540453110d8e5a66146aa9cf1b Mon Sep 17 00:00:00 2001 From: tbain Date: Thu, 26 Mar 2026 10:53:04 -0700 Subject: [PATCH 04/17] feat: #253 Fixing pylint issues --- src/openedx_tagging/models/base.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/openedx_tagging/models/base.py b/src/openedx_tagging/models/base.py index 6167f79e9..d010f0614 100644 --- a/src/openedx_tagging/models/base.py +++ b/src/openedx_tagging/models/base.py @@ -642,6 +642,13 @@ def _get_filtered_tags_deep( return qs # type: ignore[return-value] def add_counts_query(self, qs: models.QuerySet ): + """ + Adds a subquery to the passed-in queryset that returns the usage_count + for a given tag, or the appropriate count with de-deuplication per Object + for the parents of a used child tag + :param qs: The QuerySet to annotate with usage counts. + :return: the queryset annotated with the usage counts + """ # Adds a subquery to the passed-in queryset that returns the number # of times a tag has been used. # @@ -661,7 +668,7 @@ def add_counts_query(self, qs: models.QuerySet ): # build a list of lineage paths to be used in the query, so we're not hard coding to # a certain number of levels. This will build an array containing something like: # ['tag_id', 'tag__parent_id', 'tag__parent__parent_id', 'tag__parent__parent__parent_id', ...] - lineage_paths = [f"tag{'__parent' * i}_id" for i in range(0, TAXONOMY_MAX_DEPTH+1)] + lineage_paths = [f"tag{'__parent' * i}_id" for i in range(TAXONOMY_MAX_DEPTH+1)] # Combine the above-built lineage with a Q query against the OuterRef("pk"), lineage_query_list = [Q(**{path: models.OuterRef("pk")}) for path in lineage_paths] From 3df68ab5542d573dc6390eebe7c3b155d766c315 Mon Sep 17 00:00:00 2001 From: tbain Date: Thu, 26 Mar 2026 11:03:47 -0700 Subject: [PATCH 05/17] feat: #253 Fixing pycodestyle issue --- src/openedx_tagging/models/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openedx_tagging/models/base.py b/src/openedx_tagging/models/base.py index d010f0614..7e5f58398 100644 --- a/src/openedx_tagging/models/base.py +++ b/src/openedx_tagging/models/base.py @@ -641,7 +641,7 @@ def _get_filtered_tags_deep( return qs # type: ignore[return-value] - def add_counts_query(self, qs: models.QuerySet ): + def add_counts_query(self, qs: models.QuerySet): """ Adds a subquery to the passed-in queryset that returns the usage_count for a given tag, or the appropriate count with de-deuplication per Object From 435808c3e38a683263f151d656a1824620fac7e1 Mon Sep 17 00:00:00 2001 From: tbain Date: Thu, 26 Mar 2026 11:11:01 -0700 Subject: [PATCH 06/17] feat: #253 Fixing pycodestyle issue --- tests/openedx_tagging/test_models.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/openedx_tagging/test_models.py b/tests/openedx_tagging/test_models.py index c9df5255c..2473de261 100644 --- a/tests/openedx_tagging/test_models.py +++ b/tests/openedx_tagging/test_models.py @@ -788,7 +788,6 @@ def test_usage_count_with_search_term_deep(self) -> None: " Eubacteria (Bacteria) (used: 1, children: 0)", ] - def test_tree_sort(self) -> None: """ Verify that taxonomies can be sorted correctly in tree orer (case insensitive). From 457313b84ed25fa07da20abecd66854081c548b9 Mon Sep 17 00:00:00 2001 From: tbain Date: Fri, 27 Mar 2026 15:35:35 -0700 Subject: [PATCH 07/17] feat: #253 Addressing first round Code review comments --- src/openedx_tagging/models/base.py | 21 +++++++++++---------- tests/openedx_tagging/test_models.py | 9 --------- 2 files changed, 11 insertions(+), 19 deletions(-) diff --git a/src/openedx_tagging/models/base.py b/src/openedx_tagging/models/base.py index 7e5f58398..ddcbd3351 100644 --- a/src/openedx_tagging/models/base.py +++ b/src/openedx_tagging/models/base.py @@ -541,7 +541,7 @@ def _get_filtered_tags_one_level( qs = qs.values("value", "child_count", "descendant_count", "depth", "parent_value", "external_id", "_id") qs = qs.order_by("value") if include_counts: - qs = self.add_counts_query(qs) + qs = self._add_counts_query(qs) return qs # type: ignore[return-value] def _get_filtered_tags_deep( @@ -637,17 +637,18 @@ def _get_filtered_tags_deep( # ordering by it gives the tree sort order that we want. qs = qs.order_by("lineage") if include_counts: - qs = self.add_counts_query(qs) + qs = self._add_counts_query(qs) return qs # type: ignore[return-value] - def add_counts_query(self, qs: models.QuerySet): + def _add_counts_query(self, qs: models.QuerySet) -> models.QuerySet: """ Adds a subquery to the passed-in queryset that returns the usage_count - for a given tag, or the appropriate count with de-deuplication per Object - for the parents of a used child tag - :param qs: The QuerySet to annotate with usage counts. - :return: the queryset annotated with the usage counts + for a given tag, or the appropriate count with deduplication per Object + for the parents of a used child tag. + + The ``qs`` argument is the QuerySet to annotate with usage counts, and + the returned queryset is annotated with those usage counts. """ # Adds a subquery to the passed-in queryset that returns the number # of times a tag has been used. @@ -668,7 +669,8 @@ def add_counts_query(self, qs: models.QuerySet): # build a list of lineage paths to be used in the query, so we're not hard coding to # a certain number of levels. This will build an array containing something like: # ['tag_id', 'tag__parent_id', 'tag__parent__parent_id', 'tag__parent__parent__parent_id', ...] - lineage_paths = [f"tag{'__parent' * i}_id" for i in range(TAXONOMY_MAX_DEPTH+1)] + max_depth = qs.aggregate(models.Max("depth", default=0))["depth__max"] + lineage_paths = [f"tag{'__parent' * i}_id" for i in range(max_depth + 1)] # Combine the above-built lineage with a Q query against the OuterRef("pk"), lineage_query_list = [Q(**{path: models.OuterRef("pk")}) for path in lineage_paths] @@ -683,8 +685,7 @@ def add_counts_query(self, qs: models.QuerySet): # ... # ``` # Previously the above was hard coded and needed to be changed with every - # change in TAXONOMY_MAX_DEPTH, now it is dynamic to reduce maintenace - # (Thanks Google for helping me build this) + # change in TAXONOMY_MAX_DEPTH, now it is built dynamically reduce(operator.or_, lineage_query_list) ).values('object_id').distinct().annotate( diff --git a/tests/openedx_tagging/test_models.py b/tests/openedx_tagging/test_models.py index 2473de261..e7a4e5e34 100644 --- a/tests/openedx_tagging/test_models.py +++ b/tests/openedx_tagging/test_models.py @@ -596,7 +596,6 @@ def test_usage_count_lineage_count_across_same_course(self) -> None: def test_usage_count_rolls_up_to_ancestors_deep(self) -> None: """ - AI/Claude4.6 generated via IntelliJ IDEA AI Assistant When a child tag (depth 3) is applied to an object, it should roll up the count to all its ancestors when using _get_filtered_tags_deep. The child tag and each of its ancestors should have usage_count=1. @@ -629,7 +628,6 @@ def test_usage_count_rolls_up_to_ancestors_deep(self) -> None: def test_usage_count_multiple_objects_same_tag_deep(self) -> None: """ - AI/Claude4.6 generated via IntelliJ IDEA AI Assistant When two distinct objects (e.g. seperate courses, modules, etc.) are tagged with the same child tag, it should count 2 for that tag (and roll up 2 to ancestors). Each distinct object should contribute exactly 1 to the count. @@ -647,7 +645,6 @@ def test_usage_count_multiple_objects_same_tag_deep(self) -> None: def test_usage_count_sibling_tags_same_object_deduplication_deep(self) -> None: """ - AI/Claude4.6 generated via IntelliJ IDEA AI Assistant When one object is tagged with two sibling tags (both children of the same parent), the parent's usage_count should be 1, not 2. It should de-duplicate. """ @@ -666,7 +663,6 @@ def test_usage_count_sibling_tags_same_object_deduplication_deep(self) -> None: def test_usage_count_sibling_tags_different_objects_deep(self) -> None: """ - AI/Claude4.6 generated via IntelliJ IDEA AI Assistant When two different objects are each tagged with a different sibling tag, the parent's usage_count should be 2, not 1. """ @@ -683,7 +679,6 @@ def test_usage_count_sibling_tags_different_objects_deep(self) -> None: def test_usage_count_one_level_root_tags(self) -> None: """ - AI/Claude4.6 generated via IntelliJ IDEA AI Assistant _get_filtered_tags_one_level (depth=1) with include_counts=True should reflect the rolled-up usage count, not just direct usage. Tagging an object with a child tag should increment the root tag's count. @@ -700,7 +695,6 @@ def test_usage_count_one_level_root_tags(self) -> None: def test_usage_count_one_level_child_tags(self) -> None: """ - AI/Claude4.6 generated via IntelliJ IDEA AI Assistant When listing children of a tag (depth=1, parent_tag_value=...), the usage_count of each child should only reflect the objects tagged with that child or any of its descendants. @@ -722,7 +716,6 @@ def test_usage_count_one_level_child_tags(self) -> None: def test_usage_count_three_levels_deep_rollup(self) -> None: """ - AI/Claude4.6 generated via IntelliJ IDEA AI Assistant Tagging an object with a depth-3 tag (Chordata) should roll up to grandparent (Animalia) and great-grandparent (Eukaryota), verifying the full 3-level lineage query in add_counts_query. @@ -740,7 +733,6 @@ def test_usage_count_three_levels_deep_rollup(self) -> None: def test_usage_count_returns_zero_not_none_deep(self) -> None: """ - AI/Claude4.6 generated via IntelliJ IDEA AI Assistant When no object has been tagged with a tag or any of its descendants, usage_count must be 0 (integer), not None. """ @@ -771,7 +763,6 @@ def test_usage_count_returns_zero_not_none_deep(self) -> None: def test_usage_count_with_search_term_deep(self) -> None: """ - AI/Claude4.6 generated via IntelliJ IDEA AI Assistant When using get_filtered_tags() with both a search_term and include_counts=True, the usage_count returned should still reflect the true count for each matching tag, not be affected From a14c56e1b4c3f1c2ff018db0eac34276b4101757 Mon Sep 17 00:00:00 2001 From: tbain Date: Fri, 27 Mar 2026 16:54:04 -0700 Subject: [PATCH 08/17] feat: #253 fixing count depth issue and updating appropriate unit tests --- src/openedx_tagging/models/base.py | 9 +++++---- tests/openedx_tagging/test_models.py | 11 +++++++---- tests/openedx_tagging/test_views.py | 3 ++- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/openedx_tagging/models/base.py b/src/openedx_tagging/models/base.py index ddcbd3351..f53e9952f 100644 --- a/src/openedx_tagging/models/base.py +++ b/src/openedx_tagging/models/base.py @@ -665,11 +665,12 @@ def _add_counts_query(self, qs: models.QuerySet) -> models.QuerySet: # a count per tag, annotated to that particular tag from the passed-in # queryset. - # Since Depth may change depending on the value of TAXONOMY_MAX_DEPTH, dynamically - # build a list of lineage paths to be used in the query, so we're not hard coding to - # a certain number of levels. This will build an array containing something like: + # Since Depth may be variable based on the taxonomy, we dynamically build + # a list of lineage paths to be used in the query, so we're not hard coding to + # a certain number of levels. This will query for the max depth, then build + # an array containing something like: # ['tag_id', 'tag__parent_id', 'tag__parent__parent_id', 'tag__parent__parent__parent_id', ...] - max_depth = qs.aggregate(models.Max("depth", default=0))["depth__max"] + max_depth = Tag.objects.aggregate(models.Max("depth", default=0))["depth__max"] lineage_paths = [f"tag{'__parent' * i}_id" for i in range(max_depth + 1)] # Combine the above-built lineage with a Q query against the OuterRef("pk"), diff --git a/tests/openedx_tagging/test_models.py b/tests/openedx_tagging/test_models.py index e7a4e5e34..07b5c9821 100644 --- a/tests/openedx_tagging/test_models.py +++ b/tests/openedx_tagging/test_models.py @@ -417,10 +417,12 @@ def test_depth_1_queries(self) -> None: """ with self.assertNumQueries(1): self.test_get_root() - with self.assertNumQueries(1): + + # 2 queries including a query to get the max depth for tag counts + with self.assertNumQueries(2): self.test_get_depth_1_search_term() # When listing the tags below a specific tag, there is one additional query to load the parent tag: - with self.assertNumQueries(2): + with self.assertNumQueries(3): self.test_get_child_tags_one_level() with self.assertNumQueries(2): self.test_get_depth_1_child_search_term() @@ -525,8 +527,9 @@ def test_deep_queries(self) -> None: """ with self.assertNumQueries(1): self.test_get_all() - # Searching below a specific tag requires an additional query to load that tag: - with self.assertNumQueries(2): + # Searching below a specific tag requires an additional query to load that tag, + # 3 queries including a query to get the max depth for tag counts: + with self.assertNumQueries(3): self.test_tags_deep() # Keyword search requires an additional query: with self.assertNumQueries(2): diff --git a/tests/openedx_tagging/test_views.py b/tests/openedx_tagging/test_views.py index 63de07155..eca1b459a 100644 --- a/tests/openedx_tagging/test_views.py +++ b/tests/openedx_tagging/test_views.py @@ -1614,7 +1614,8 @@ def test_large_taxonomy(self): self.client.force_authenticate(user=self.staff) url = self.large_taxonomy_url + "?include_counts" - with self.assertNumQueries(3): + # 4 queries, including 1 for max depth for counts + with self.assertNumQueries(4): response = self.client.get(url) assert response.status_code == status.HTTP_200_OK From 2055a0752274d68083174582f2e9822b97dc2271 Mon Sep 17 00:00:00 2001 From: tbain Date: Fri, 27 Mar 2026 16:56:45 -0700 Subject: [PATCH 09/17] feat: #253 fixing spelling errors in comments --- tests/openedx_tagging/test_models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/openedx_tagging/test_models.py b/tests/openedx_tagging/test_models.py index 07b5c9821..0b7f9ca8f 100644 --- a/tests/openedx_tagging/test_models.py +++ b/tests/openedx_tagging/test_models.py @@ -550,7 +550,7 @@ def test_get_external_id(self) -> None: def test_usage_count(self) -> None: """ Test that the usage count in the results is right for a basic case; - many objects tagged seperately should return a simple usage count that + many objects tagged separately should return a simple usage count that reflects lineage de-duplication (or lack thereof, in this case) """ api.tag_object(object_id="obj01", taxonomy=self.taxonomy, tags=["Bacteria"]) @@ -631,7 +631,7 @@ def test_usage_count_rolls_up_to_ancestors_deep(self) -> None: def test_usage_count_multiple_objects_same_tag_deep(self) -> None: """ - When two distinct objects (e.g. seperate courses, modules, etc.) are tagged + When two distinct objects (e.g. separate courses, modules, etc.) are tagged with the same child tag, it should count 2 for that tag (and roll up 2 to ancestors). Each distinct object should contribute exactly 1 to the count. """ From 939f18c8950246d575caba1fbd876a1472cd65ab Mon Sep 17 00:00:00 2001 From: tbain Date: Mon, 30 Mar 2026 14:50:53 -0700 Subject: [PATCH 10/17] feat: #253 Fixing code review comments; fix incorrect unit test & filter query to current taxonomy --- src/openedx_tagging/models/base.py | 10 +++++----- tests/openedx_tagging/test_models.py | 8 ++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/openedx_tagging/models/base.py b/src/openedx_tagging/models/base.py index f53e9952f..922fab07f 100644 --- a/src/openedx_tagging/models/base.py +++ b/src/openedx_tagging/models/base.py @@ -670,14 +670,15 @@ def _add_counts_query(self, qs: models.QuerySet) -> models.QuerySet: # a certain number of levels. This will query for the max depth, then build # an array containing something like: # ['tag_id', 'tag__parent_id', 'tag__parent__parent_id', 'tag__parent__parent__parent_id', ...] - max_depth = Tag.objects.aggregate(models.Max("depth", default=0))["depth__max"] + max_depth = Tag.objects.filter(taxonomy_id=self.id).aggregate(models.Max("depth", default=0))["depth__max"] lineage_paths = [f"tag{'__parent' * i}_id" for i in range(max_depth + 1)] - # Combine the above-built lineage with a Q query against the OuterRef("pk"), lineage_query_list = [Q(**{path: models.OuterRef("pk")}) for path in lineage_paths] usage_count_qs = ObjectTag.objects.filter( - # Combine the logic built above with an or operator to flesh out a + taxonomy_id=self.id + ).filter( + # Combine the logic built above with an or operator to build out a # lineage query of the form: # ``` # Q(tag_id=OuterRef('pk')) | @@ -687,8 +688,7 @@ def _add_counts_query(self, qs: models.QuerySet) -> models.QuerySet: # ``` # Previously the above was hard coded and needed to be changed with every # change in TAXONOMY_MAX_DEPTH, now it is built dynamically - - reduce(operator.or_, lineage_query_list) + reduce(operator.or_, lineage_query_list), ).values('object_id').distinct().annotate( intermediate_grouping=Value(1, output_field=IntegerField()) ).values('intermediate_grouping').annotate( diff --git a/tests/openedx_tagging/test_models.py b/tests/openedx_tagging/test_models.py index 0b7f9ca8f..0f42b1d40 100644 --- a/tests/openedx_tagging/test_models.py +++ b/tests/openedx_tagging/test_models.py @@ -578,15 +578,15 @@ def test_usage_count_lineage_count_across_same_course(self) -> None: child tags being added to an object. However, we de-duplicate and only count 1 parent tag towards a course even if 2 children are applied to that course """ - api.tag_object(object_id="obj01", taxonomy=self.taxonomy, tags=["Bacteria"]) - api.tag_object(object_id="obj01", taxonomy=self.taxonomy, tags=["Archaebacteria"]) + self.taxonomy.allow_multiple = True + self.taxonomy.save() + api.tag_object(object_id="obj01", taxonomy=self.taxonomy, tags=["Bacteria", "Archaebacteria", "Eubacteria"]) api.tag_object(object_id="obj02", taxonomy=self.taxonomy, tags=["Archaebacteria"]) - api.tag_object(object_id="obj01", taxonomy=self.taxonomy, tags=["Eubacteria"]) # Now the API should reflect these usage counts: result = pretty_format_tags(self.taxonomy.get_filtered_tags(search_term="bacteria", include_counts=True)) assert result == [ "Bacteria (None) (used: 2, children: 2)", - " Archaebacteria (Bacteria) (used: 1, children: 0)", + " Archaebacteria (Bacteria) (used: 2, children: 0)", " Eubacteria (Bacteria) (used: 1, children: 0)", ] # Same with depth=1, which uses a different query internally: From 5762c3313e6222a0e2ea5523dd34e196ac5e1d14 Mon Sep 17 00:00:00 2001 From: tbain Date: Wed, 1 Apr 2026 11:37:04 -0700 Subject: [PATCH 11/17] feat: #253 adjusting comments per code review feedback --- tests/openedx_tagging/test_api.py | 2 +- tests/openedx_tagging/test_models.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/openedx_tagging/test_api.py b/tests/openedx_tagging/test_api.py index 3f0d08e69..00a5420f2 100644 --- a/tests/openedx_tagging/test_api.py +++ b/tests/openedx_tagging/test_api.py @@ -789,7 +789,7 @@ def get_object_tags(): "Eukaryota (used: 6, children: 4 + 8)", " Animalia (used: 4, children: 7 + 1)", " Arthropoda (used: 1, children: 0)", - " Chordata (used: 0, children: 1)", # <<< Chordata has a matching child but we only support searching + " Chordata (used: 0, children: 1)", " Mammalia (used: 0, children: 0)", " Cnidaria (used: 0, children: 0)", # 3 levels deep at once for now. " Ctenophora (used: 0, children: 0)", diff --git a/tests/openedx_tagging/test_models.py b/tests/openedx_tagging/test_models.py index 0f42b1d40..fc6121237 100644 --- a/tests/openedx_tagging/test_models.py +++ b/tests/openedx_tagging/test_models.py @@ -576,7 +576,7 @@ def test_usage_count_lineage_count_across_same_course(self) -> None: """ Test that the usage count is correct and parent counts are included based on child tags being added to an object. However, we de-duplicate and only count - 1 parent tag towards a course even if 2 children are applied to that course + 1 parent tag towards each object even if 2 children are applied to that object """ self.taxonomy.allow_multiple = True self.taxonomy.save() From c2f79d21092163e9df2b4c6d359356a8fe744251 Mon Sep 17 00:00:00 2001 From: tbain Date: Wed, 1 Apr 2026 12:34:22 -0700 Subject: [PATCH 12/17] feat: #253 fixing unit tests to work with upstream updates --- tests/openedx_tagging/test_api.py | 6 +++--- tests/openedx_tagging/test_models.py | 14 +++++++------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/openedx_tagging/test_api.py b/tests/openedx_tagging/test_api.py index cde3b1b82..ad7751d58 100644 --- a/tests/openedx_tagging/test_api.py +++ b/tests/openedx_tagging/test_api.py @@ -764,7 +764,7 @@ def get_object_tags(): " Proteoarchaeota (used: 0, children: 0)", "Bacteria (used: 1, children: 1)", # does not contain "ar" but a child does " Archaebacteria (used: 1, children: 0)", - "Eukaryota (used: 6, children: 1 + 2)", + "Eukaryota (used: 6, children: 1)", " Animalia (used: 4, children: 2)", # does not contain "ar" but a child does " Arthropoda (used: 1, children: 0)", " Cnidaria (used: 0, children: 0)", @@ -786,8 +786,8 @@ def get_object_tags(): "Bacteria (used: 1, children: 2)", " Archaebacteria (used: 1, children: 0)", " Eubacteria (used: 0, children: 0)", - "Eukaryota (used: 6, children: 4 + 8)", - " Animalia (used: 4, children: 7 + 1)", + "Eukaryota (used: 6, children: 4)", + " Animalia (used: 4, children: 7)", " Arthropoda (used: 1, children: 0)", " Chordata (used: 0, children: 1)", " Mammalia (used: 0, children: 0)", diff --git a/tests/openedx_tagging/test_models.py b/tests/openedx_tagging/test_models.py index 1e29a4dfc..2ead1e421 100644 --- a/tests/openedx_tagging/test_models.py +++ b/tests/openedx_tagging/test_models.py @@ -610,8 +610,8 @@ def test_usage_count_rolls_up_to_ancestors_deep(self) -> None: "Bacteria (None) (used: 0, children: 2)", " Archaebacteria (Bacteria) (used: 0, children: 0)", " Eubacteria (Bacteria) (used: 0, children: 0)", - "Eukaryota (None) (used: 1, children: 5 + 8)", - " Animalia (Eukaryota) (used: 1, children: 7 + 1)", + "Eukaryota (None) (used: 1, children: 5)", + " Animalia (Eukaryota) (used: 1, children: 7)", " Arthropoda (Animalia) (used: 0, children: 0)", " Chordata (Animalia) (used: 1, children: 1)", " Mammalia (Chordata) (used: 1, children: 0)", @@ -638,7 +638,7 @@ def test_usage_count_multiple_objects_same_tag_deep(self) -> None: self.taxonomy.get_filtered_tags(search_term="chordata", include_counts=True) ) assert result == [ - "Eukaryota (None) (used: 2, children: 1 + 1)", + "Eukaryota (None) (used: 2, children: 1)", " Animalia (Eukaryota) (used: 2, children: 1)", " Chordata (Animalia) (used: 2, children: 0)", ] @@ -690,7 +690,7 @@ def test_usage_count_one_level_root_tags(self) -> None: assert result == [ "Archaea (None) (used: 0, children: 3)", "Bacteria (None) (used: 1, children: 2)", - "Eukaryota (None) (used: 0, children: 5 + 8)", + "Eukaryota (None) (used: 0, children: 5)", ] def test_usage_count_one_level_child_tags(self) -> None: @@ -726,7 +726,7 @@ def test_usage_count_three_levels_deep_rollup(self) -> None: self.taxonomy.get_filtered_tags(search_term="chordata", include_counts=True) ) assert result == [ - "Eukaryota (None) (used: 1, children: 1 + 1)", + "Eukaryota (None) (used: 1, children: 1)", " Animalia (Eukaryota) (used: 1, children: 1)", " Chordata (Animalia) (used: 1, children: 0)", ] @@ -745,8 +745,8 @@ def test_usage_count_returns_zero_not_none_deep(self) -> None: "Bacteria (None) (used: 0, children: 2)", " Archaebacteria (Bacteria) (used: 0, children: 0)", " Eubacteria (Bacteria) (used: 0, children: 0)", - "Eukaryota (None) (used: 0, children: 5 + 8)", - " Animalia (Eukaryota) (used: 0, children: 7 + 1)", + "Eukaryota (None) (used: 0, children: 5)", + " Animalia (Eukaryota) (used: 0, children: 7)", " Arthropoda (Animalia) (used: 0, children: 0)", " Chordata (Animalia) (used: 0, children: 1)", " Mammalia (Chordata) (used: 0, children: 0)", From c017e8ab1581911c800d315ec5b888c08e72a08a Mon Sep 17 00:00:00 2001 From: tbain Date: Fri, 3 Apr 2026 13:52:19 -0700 Subject: [PATCH 13/17] feat: #253 Changing usage_count to being in-mem/python based instead of via expensive db query --- src/openedx_tagging/models/base.py | 100 +++++++++------------------ tests/openedx_tagging/test_models.py | 11 +-- 2 files changed, 40 insertions(+), 71 deletions(-) diff --git a/src/openedx_tagging/models/base.py b/src/openedx_tagging/models/base.py index 975ad0d70..535a44070 100644 --- a/src/openedx_tagging/models/base.py +++ b/src/openedx_tagging/models/base.py @@ -5,15 +5,15 @@ from __future__ import annotations import logging -import operator import re -from functools import reduce + from typing import List, Self +from collections import Counter, defaultdict from django.core.exceptions import ValidationError from django.db import models -from django.db.models import Count, F, IntegerField, Q, Subquery, Value -from django.db.models.functions import Coalesce, Concat, Length, Replace, Substr +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 _ @@ -534,7 +534,8 @@ def _get_filtered_tags_one_level( qs = qs.values("value", "child_count", "depth", "parent_value", "external_id", "_id") qs = qs.order_by("value") if include_counts: - qs = self._add_counts_query(qs) + return self._add_counts(list(qs)) # type: ignore[return-value] + return qs # type: ignore[return-value] def _get_filtered_tags_deep( @@ -609,71 +610,38 @@ def _get_filtered_tags_deep( # ordering by it gives the tree sort order that we want. qs = qs.order_by("lineage") if include_counts: - qs = self._add_counts_query(qs) + return self._add_counts(list(qs)) # type: ignore[return-value] return qs # type: ignore[return-value] - def _add_counts_query(self, qs: models.QuerySet) -> models.QuerySet: - """ - Adds a subquery to the passed-in queryset that returns the usage_count - for a given tag, or the appropriate count with deduplication per Object - for the parents of a used child tag. - - The ``qs`` argument is the QuerySet to annotate with usage counts, and - the returned queryset is annotated with those usage counts. - """ - # Adds a subquery to the passed-in queryset that returns the number - # of times a tag has been used. - # - # Note: The count is not a simple count, we need to do a 'roll up' - # where we count the number of times a tag is directly used and applied, - # but then that also needs to add a "1" count to the lineage tags - # (parent, grandparent, etc.), but de-duplicate counts for any children - # so that if we have "2" child tags, it only counts towards "1" for the - # parent. - # This query gets the raw counts for each tag usage, gets the distinct - # usages (so de-duplicates counts) by actual application to an "Object" - # (library, course, course module, course section, etc.), which creates - # a count per tag, annotated to that particular tag from the passed-in - # queryset. - - # Since Depth may be variable based on the taxonomy, we dynamically build - # a list of lineage paths to be used in the query, so we're not hard coding to - # a certain number of levels. This will query for the max depth, then build - # an array containing something like: - # ['tag_id', 'tag__parent_id', 'tag__parent__parent_id', 'tag__parent__parent__parent_id', ...] - max_depth = Tag.objects.filter(taxonomy_id=self.id).aggregate(models.Max("depth", default=0))["depth__max"] - lineage_paths = [f"tag{'__parent' * i}_id" for i in range(max_depth + 1)] - # Combine the above-built lineage with a Q query against the OuterRef("pk"), - lineage_query_list = [Q(**{path: models.OuterRef("pk")}) for path in lineage_paths] - - usage_count_qs = ObjectTag.objects.filter( - taxonomy_id=self.id - ).filter( - # Combine the logic built above with an or operator to build out a - # lineage query of the form: - # ``` - # Q(tag_id=OuterRef('pk')) | - # Q(tag__parent_id=OuterRef('pk')) | - # Q(tag__parent__parent_id=OuterRef('pk')) | - # ... - # ``` - # Previously the above was hard coded and needed to be changed with every - # change in TAXONOMY_MAX_DEPTH, now it is built dynamically - reduce(operator.or_, lineage_query_list), - ).values('object_id').distinct().annotate( - intermediate_grouping=Value(1, output_field=IntegerField()) - ).values('intermediate_grouping').annotate( - total_usage=Count('object_id', distinct=True) - ).values('total_usage') + def _add_counts(self, tag_data: list[dict]) -> list[dict]: + """ + Add usage counts to a list of tag data dictionaries. For performance + reasons, we call this function with the list result of the + QuerySet so we can then add the counts in-memory rather than to a + QuerySet which would require a very expensive annotation to join the + in-memory data to the original QuerySet. + """ - qs = qs.annotate( - usage_count=Coalesce( - Subquery(usage_count_qs, output_field=IntegerField()), - 0 # Coalesce ensures we return 0 instead of None if there are no usages - ) - ) - return qs + tag_lineage_dict = dict(self.tag_set.all().filter(taxonomy_id=self.id).values_list("value", "lineage")) + object_tags = self.objecttag_set.all().filter(taxonomy_id=self.id).values_list("_value", "object_id") + tag_counts = Counter() + object_tag_lineage_seen = defaultdict(set) + + for tag_value, object_id in object_tags: + # split the lineages to get a dict of {tag.value: [lineages]} + lineage_tags = (t for t in tag_lineage_dict.get(tag_value, "").split('\t') if t) + # de-duplicate based on if the lineage is already 'seen' per object + unseen_tags = [t for t in lineage_tags if t not in object_tag_lineage_seen[object_id]] + + tag_counts.update(unseen_tags) + object_tag_lineage_seen[object_id].update(unseen_tags) + + # In-memory 'annotation'; this is faster than using annotate() on the QuerySet. + for row in tag_data: + row["usage_count"] = tag_counts.get(row["value"], 0) + + return tag_data def add_tag( self, diff --git a/tests/openedx_tagging/test_models.py b/tests/openedx_tagging/test_models.py index 2ead1e421..8f801c77f 100644 --- a/tests/openedx_tagging/test_models.py +++ b/tests/openedx_tagging/test_models.py @@ -416,11 +416,11 @@ def test_depth_1_queries(self) -> None: with self.assertNumQueries(1): self.test_get_root() - # 2 queries including a query to get the max depth for tag counts - with self.assertNumQueries(2): + # 2 queries including a query to get 1. max depth and 2. objs for tag counts + with self.assertNumQueries(3): self.test_get_depth_1_search_term() # When listing the tags below a specific tag, there is one additional query to load the parent tag: - with self.assertNumQueries(3): + with self.assertNumQueries(4): self.test_get_child_tags_one_level() with self.assertNumQueries(2): self.test_get_depth_1_child_search_term() @@ -525,8 +525,9 @@ def test_deep_queries(self) -> None: with self.assertNumQueries(1): self.test_get_all() # Searching below a specific tag requires an additional query to load that tag, - # 3 queries including a query to get the max depth for tag counts: - with self.assertNumQueries(3): + # 4 queries including a query to get the 1.max depth for tag counts and 2 objs + # for counts: + with self.assertNumQueries(4): self.test_tags_deep() # Keyword search requires an additional query: with self.assertNumQueries(2): From 7d42793f3e0a5d3b83d8b0780a36c80c91f1d4b9 Mon Sep 17 00:00:00 2001 From: tbain Date: Fri, 3 Apr 2026 14:36:05 -0700 Subject: [PATCH 14/17] feat: #253 Fixing code quality pipeline issues --- src/openedx_tagging/models/base.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/openedx_tagging/models/base.py b/src/openedx_tagging/models/base.py index 535a44070..882a4ec3f 100644 --- a/src/openedx_tagging/models/base.py +++ b/src/openedx_tagging/models/base.py @@ -6,9 +6,8 @@ import logging import re - -from typing import List, Self from collections import Counter, defaultdict +from typing import List, Self, cast from django.core.exceptions import ValidationError from django.db import models @@ -534,7 +533,7 @@ def _get_filtered_tags_one_level( qs = qs.values("value", "child_count", "depth", "parent_value", "external_id", "_id") qs = qs.order_by("value") if include_counts: - return self._add_counts(list(qs)) # type: ignore[return-value] + return self._add_counts(list(cast(list, qs))) # type: ignore[return-value] return qs # type: ignore[return-value] @@ -610,7 +609,7 @@ def _get_filtered_tags_deep( # ordering by it gives the tree sort order that we want. qs = qs.order_by("lineage") if include_counts: - return self._add_counts(list(qs)) # type: ignore[return-value] + return self._add_counts(list(cast(list, qs))) # type: ignore[return-value] return qs # type: ignore[return-value] @@ -625,8 +624,8 @@ def _add_counts(self, tag_data: list[dict]) -> list[dict]: tag_lineage_dict = dict(self.tag_set.all().filter(taxonomy_id=self.id).values_list("value", "lineage")) object_tags = self.objecttag_set.all().filter(taxonomy_id=self.id).values_list("_value", "object_id") - tag_counts = Counter() - object_tag_lineage_seen = defaultdict(set) + tag_counts: Counter[str] = Counter() + object_tag_lineage_seen: defaultdict[str, set] = defaultdict(set) for tag_value, object_id in object_tags: # split the lineages to get a dict of {tag.value: [lineages]} From 1e47967222bf3fe980dcbee24f7744b11ec8eaa3 Mon Sep 17 00:00:00 2001 From: tbain Date: Wed, 8 Apr 2026 16:14:38 -0700 Subject: [PATCH 15/17] feat: #253 Moving usage_count logic out to API level, cleaning up/adding associated unit tests --- src/openedx_tagging/models/base.py | 48 +-- src/openedx_tagging/rest_api/v1/views.py | 41 ++- tests/openedx_tagging/test_api.py | 82 ++--- tests/openedx_tagging/test_models.py | 278 +-------------- tests/openedx_tagging/test_views.py | 422 ++++++++++++++++++++++- 5 files changed, 517 insertions(+), 354 deletions(-) diff --git a/src/openedx_tagging/models/base.py b/src/openedx_tagging/models/base.py index 882a4ec3f..046219ed3 100644 --- a/src/openedx_tagging/models/base.py +++ b/src/openedx_tagging/models/base.py @@ -6,8 +6,7 @@ import logging import re -from collections import Counter, defaultdict -from typing import List, Self, cast +from typing import List, Self from django.core.exceptions import ValidationError from django.db import models @@ -480,7 +479,7 @@ def get_filtered_tags( # pylint: disable=too-many-positional-arguments def _get_filtered_tags_free_text( self, search_term: str | None, - include_counts: bool, + include_counts: bool, # pylint: disable=unused-argument ) -> TagDataQuerySet: """ Implementation of get_filtered_tags() for free text taxonomies. @@ -500,16 +499,14 @@ def _get_filtered_tags_free_text( _id=Value(None, output_field=models.CharField()), ) qs = qs.values("value", "child_count", "depth", "parent_value", "external_id", "_id").order_by("value") - if include_counts: - return qs.annotate(usage_count=models.Count("value")) - else: - return qs.distinct() # type: ignore[return-value] + + return qs.distinct() # type: ignore[return-value] def _get_filtered_tags_one_level( self, parent_tag_value: str | None, search_term: str | None, - include_counts: bool, + include_counts: bool, # pylint: disable=unused-argument ) -> TagDataQuerySet: """ Implementation of get_filtered_tags() for closed taxonomies, where @@ -532,8 +529,6 @@ def _get_filtered_tags_one_level( qs = qs.annotate(_id=F("id")) # ID has an underscore to encourage use of 'value' rather than this internal ID qs = qs.values("value", "child_count", "depth", "parent_value", "external_id", "_id") qs = qs.order_by("value") - if include_counts: - return self._add_counts(list(cast(list, qs))) # type: ignore[return-value] return qs # type: ignore[return-value] @@ -541,7 +536,7 @@ def _get_filtered_tags_deep( self, parent_tag_value: str | None, search_term: str | None, - include_counts: bool, + include_counts: bool, # pylint: disable=unused-argument excluded_values: list[str] | None, ) -> TagDataQuerySet: """ @@ -608,40 +603,9 @@ def _get_filtered_tags_deep( # lineage is a case-insensitive column storing "Root\tParent\t...\tThisValue\t", so # ordering by it gives the tree sort order that we want. qs = qs.order_by("lineage") - if include_counts: - return self._add_counts(list(cast(list, qs))) # type: ignore[return-value] return qs # type: ignore[return-value] - def _add_counts(self, tag_data: list[dict]) -> list[dict]: - """ - Add usage counts to a list of tag data dictionaries. For performance - reasons, we call this function with the list result of the - QuerySet so we can then add the counts in-memory rather than to a - QuerySet which would require a very expensive annotation to join the - in-memory data to the original QuerySet. - """ - - tag_lineage_dict = dict(self.tag_set.all().filter(taxonomy_id=self.id).values_list("value", "lineage")) - object_tags = self.objecttag_set.all().filter(taxonomy_id=self.id).values_list("_value", "object_id") - tag_counts: Counter[str] = Counter() - object_tag_lineage_seen: defaultdict[str, set] = defaultdict(set) - - for tag_value, object_id in object_tags: - # split the lineages to get a dict of {tag.value: [lineages]} - lineage_tags = (t for t in tag_lineage_dict.get(tag_value, "").split('\t') if t) - # de-duplicate based on if the lineage is already 'seen' per object - unseen_tags = [t for t in lineage_tags if t not in object_tag_lineage_seen[object_id]] - - tag_counts.update(unseen_tags) - object_tag_lineage_seen[object_id].update(unseen_tags) - - # In-memory 'annotation'; this is faster than using annotate() on the QuerySet. - for row in tag_data: - row["usage_count"] = tag_counts.get(row["value"], 0) - - return tag_data - def add_tag( self, tag_value: str, diff --git a/src/openedx_tagging/rest_api/v1/views.py b/src/openedx_tagging/rest_api/v1/views.py index 97761286f..237bd890b 100644 --- a/src/openedx_tagging/rest_api/v1/views.py +++ b/src/openedx_tagging/rest_api/v1/views.py @@ -3,6 +3,8 @@ """ from __future__ import annotations +from collections import Counter, defaultdict + from django.core import exceptions from django.db import models from django.http import Http404, HttpResponse @@ -847,17 +849,54 @@ def get_queryset(self) -> TagDataQuerySet: ) if depth == 1: # We're already returning just a single level. It will be paginated normally. + if include_counts: + return self._add_counts(results) return results elif full_depth_threshold and len(results) < full_depth_threshold: # We can load and display all the tags in this (sub)tree at once: self.pagination_class = DisabledTagsPagination + if include_counts: + return self._add_counts(results) return results else: # We had to do a deep query, but we will only return one level of results. # This is because the user did not request a deep response (via full_depth_threshold) or the result was too # large (larger than the threshold). # It will be paginated normally. - return results.filter(parent_value=parent_tag_value) + filtered_results = results.filter(parent_value=parent_tag_value) + if include_counts: + return self._add_counts(filtered_results) + + return filtered_results + + def _add_counts(self, tag_data: TagDataQuerySet) -> TagDataQuerySet: + """ + Add usage counts to a list of tag data dictionaries. For performance + reasons, we call this function with the list result of the + QuerySet so we can then add the counts in-memory rather than to a + QuerySet which would require a very expensive annotation to join the + in-memory data to the original QuerySet. + """ + + taxonomy = self.get_taxonomy() + object_tags = taxonomy.objecttag_set.values_list("object_id", "tag__lineage") + tag_counts: Counter[str] = Counter() + object_tag_lineage_seen: defaultdict[str, set] = defaultdict(set) + + for object_id, tag_lineage in object_tags: + # split the lineages to get a dict of {tag.value: [lineages]} + lineage_tags = list(tag_lineage.split('\t')) if tag_lineage else [] + # de-duplicate based on if the lineage is already 'seen' per object + unseen_tags = [t for t in lineage_tags if t not in object_tag_lineage_seen[object_id]] + + tag_counts.update(unseen_tags) + object_tag_lineage_seen[object_id].update(unseen_tags) + + # In-memory 'annotation'; this is faster than using annotate() on the QuerySet. + for row in tag_data: + row["usage_count"] = tag_counts.get(row["value"], 0) + + return tag_data def post(self, request, *args, **kwargs): """ diff --git a/tests/openedx_tagging/test_api.py b/tests/openedx_tagging/test_api.py index ad7751d58..d3accf4b4 100644 --- a/tests/openedx_tagging/test_api.py +++ b/tests/openedx_tagging/test_api.py @@ -752,53 +752,53 @@ def get_object_tags(): @ddt.data( ("ChA", [ - "Archaea (used: 1, children: 2)", - " Euryarchaeida (used: 0, children: 0)", - " Proteoarchaeota (used: 0, children: 0)", - "Bacteria (used: 1, children: 1)", # does not contain "cha" but a child does - " Archaebacteria (used: 1, children: 0)", + "Archaea (children: 2)", + " Euryarchaeida (children: 0)", + " Proteoarchaeota (children: 0)", + "Bacteria (children: 1)", # does not contain "cha" but a child does + " Archaebacteria (children: 0)", ]), ("ar", [ - "Archaea (used: 1, children: 2)", - " Euryarchaeida (used: 0, children: 0)", - " Proteoarchaeota (used: 0, children: 0)", - "Bacteria (used: 1, children: 1)", # does not contain "ar" but a child does - " Archaebacteria (used: 1, children: 0)", - "Eukaryota (used: 6, children: 1)", - " Animalia (used: 4, children: 2)", # does not contain "ar" but a child does - " Arthropoda (used: 1, children: 0)", - " Cnidaria (used: 0, children: 0)", + "Archaea (children: 2)", + " Euryarchaeida (children: 0)", + " Proteoarchaeota (children: 0)", + "Bacteria (children: 1)", # does not contain "ar" but a child does + " Archaebacteria (children: 0)", + "Eukaryota (children: 1)", + " Animalia (children: 2)", # does not contain "ar" but a child does + " Arthropoda (children: 0)", + " Cnidaria (children: 0)", ]), ("aE", [ - "Archaea (used: 1, children: 2)", - " Euryarchaeida (used: 0, children: 0)", - " Proteoarchaeota (used: 0, children: 0)", - "Bacteria (used: 1, children: 1)", # does not contain "ae" but a child does - " Archaebacteria (used: 1, children: 0)", - "Eukaryota (used: 6, children: 1)", # does not contain "ae" but a child does - " Plantae (used: 1, children: 0)", + "Archaea (children: 2)", + " Euryarchaeida (children: 0)", + " Proteoarchaeota (children: 0)", + "Bacteria (children: 1)", # does not contain "ae" but a child does + " Archaebacteria (children: 0)", + "Eukaryota (children: 1)", # does not contain "ae" but a child does + " Plantae (children: 0)", ]), ("a", [ - "Archaea (used: 1, children: 3)", - " DPANN (used: 0, children: 0)", - " Euryarchaeida (used: 0, children: 0)", - " Proteoarchaeota (used: 0, children: 0)", - "Bacteria (used: 1, children: 2)", - " Archaebacteria (used: 1, children: 0)", - " Eubacteria (used: 0, children: 0)", - "Eukaryota (used: 6, children: 4)", - " Animalia (used: 4, children: 7)", - " Arthropoda (used: 1, children: 0)", - " Chordata (used: 0, children: 1)", - " Mammalia (used: 0, children: 0)", - " Cnidaria (used: 0, children: 0)", - " Ctenophora (used: 0, children: 0)", - " Gastrotrich (used: 1, children: 0)", - " Placozoa (used: 1, children: 0)", - " Porifera (used: 0, children: 0)", - " Monera (used: 1, children: 0)", - " Plantae (used: 1, children: 0)", - " Protista (used: 0, children: 0)", + "Archaea (children: 3)", + " DPANN (children: 0)", + " Euryarchaeida (children: 0)", + " Proteoarchaeota (children: 0)", + "Bacteria (children: 2)", + " Archaebacteria (children: 0)", + " Eubacteria (children: 0)", + "Eukaryota (children: 4)", + " Animalia (children: 7)", + " Arthropoda (children: 0)", + " Chordata (children: 1)", + " Mammalia (children: 0)", + " Cnidaria (children: 0)", + " Ctenophora (children: 0)", + " Gastrotrich (children: 0)", + " Placozoa (children: 0)", + " Porifera (children: 0)", + " Monera (children: 0)", + " Plantae (children: 0)", + " Protista (children: 0)", ]), ) @ddt.unpack diff --git a/tests/openedx_tagging/test_models.py b/tests/openedx_tagging/test_models.py index 8f801c77f..22d60d430 100644 --- a/tests/openedx_tagging/test_models.py +++ b/tests/openedx_tagging/test_models.py @@ -51,7 +51,6 @@ def setUp(self): self.chordata = get_tag("Chordata") self.mammalia = get_tag("Mammalia") self.animalia = get_tag("Animalia") - self.eukaryota = get_tag("Eukaryota") 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()( @@ -340,8 +339,8 @@ def test_get_child_tags_one_level(self) -> None: Test basic retrieval of tags one level below the "Eukaryota" root tag in the closed taxonomy, using get_filtered_tags(). With counts included. """ - result = list(self.taxonomy.get_filtered_tags(depth=1, parent_tag_value="Eukaryota", include_counts=True)) - common_fields = {"depth": 1, "parent_value": "Eukaryota", "usage_count": 0, "external_id": None} + result = list(self.taxonomy.get_filtered_tags(depth=1, parent_tag_value="Eukaryota")) + common_fields = {"depth": 1, "parent_value": "Eukaryota", "external_id": None} for r in result: del r["_id"] # Remove the internal database IDs; they aren't interesting here and a other tests check them assert result == [ @@ -377,13 +376,12 @@ def test_get_depth_1_search_term(self) -> None: """ Filter the root tags to only those that match a search term """ - result = list(self.taxonomy.get_filtered_tags(depth=1, search_term="ARCH", include_counts=True)) + result = list(self.taxonomy.get_filtered_tags(depth=1, search_term="ARCH")) assert result == [ { "value": "Archaea", "child_count": 3, "depth": 0, - "usage_count": 0, "parent_value": None, "external_id": None, "_id": 2, # These IDs are hard-coded in the test fixture file @@ -415,12 +413,10 @@ def test_depth_1_queries(self) -> None: """ with self.assertNumQueries(1): self.test_get_root() - - # 2 queries including a query to get 1. max depth and 2. objs for tag counts - with self.assertNumQueries(3): + with self.assertNumQueries(1): self.test_get_depth_1_search_term() # When listing the tags below a specific tag, there is one additional query to load the parent tag: - with self.assertNumQueries(4): + with self.assertNumQueries(2): self.test_get_child_tags_one_level() with self.assertNumQueries(2): self.test_get_depth_1_child_search_term() @@ -504,13 +500,12 @@ def test_tags_deep(self) -> None: """ Test getting a deep tag in the taxonomy """ - result = list(self.taxonomy.get_filtered_tags(parent_tag_value="Chordata", include_counts=True)) + result = list(self.taxonomy.get_filtered_tags(parent_tag_value="Chordata")) assert result == [ { "value": "Mammalia", "parent_value": "Chordata", "depth": 3, - "usage_count": 0, "child_count": 0, "external_id": None, "_id": 21, # These IDs are hard-coded in the test fixture file @@ -524,10 +519,8 @@ def test_deep_queries(self) -> None: """ with self.assertNumQueries(1): self.test_get_all() - # Searching below a specific tag requires an additional query to load that tag, - # 4 queries including a query to get the 1.max depth for tag counts and 2 objs - # for counts: - with self.assertNumQueries(4): + # Searching below a specific tag requires an additional query to load that tag: + with self.assertNumQueries(2): self.test_tags_deep() # Keyword search requires an additional query: with self.assertNumQueries(2): @@ -545,241 +538,6 @@ def test_get_external_id(self) -> None: assert result[0]["value"] == "Bacteria" assert result[0]["external_id"] == "bct001" - def test_usage_count(self) -> None: - """ - Test that the usage count in the results is right for a basic case; - many objects tagged separately should return a simple usage count that - reflects lineage de-duplication (or lack thereof, in this case) - """ - api.tag_object(object_id="obj01", taxonomy=self.taxonomy, tags=["Bacteria"]) - api.tag_object(object_id="obj02", taxonomy=self.taxonomy, tags=["Bacteria"]) - api.tag_object(object_id="obj03", taxonomy=self.taxonomy, tags=["Bacteria"]) - api.tag_object(object_id="obj04", taxonomy=self.taxonomy, tags=["Eubacteria"]) - # Now the API should reflect these usage counts: - result = pretty_format_tags(self.taxonomy.get_filtered_tags(search_term="bacteria", include_counts=True)) - assert result == [ - "Bacteria (None) (used: 4, children: 2)", - " Archaebacteria (Bacteria) (used: 0, children: 0)", - " Eubacteria (Bacteria) (used: 1, children: 0)", - ] - # Same with depth=1, which uses a different query internally: - result1 = pretty_format_tags( - self.taxonomy.get_filtered_tags(search_term="bacteria", include_counts=True, depth=1) - ) - assert result1 == [ - "Bacteria (None) (used: 4, children: 2)", - ] - - def test_usage_count_lineage_count_across_same_course(self) -> None: - """ - Test that the usage count is correct and parent counts are included based on - child tags being added to an object. However, we de-duplicate and only count - 1 parent tag towards each object even if 2 children are applied to that object - """ - self.taxonomy.allow_multiple = True - self.taxonomy.save() - api.tag_object(object_id="obj01", taxonomy=self.taxonomy, tags=["Bacteria", "Archaebacteria", "Eubacteria"]) - api.tag_object(object_id="obj02", taxonomy=self.taxonomy, tags=["Archaebacteria"]) - # Now the API should reflect these usage counts: - result = pretty_format_tags(self.taxonomy.get_filtered_tags(search_term="bacteria", include_counts=True)) - assert result == [ - "Bacteria (None) (used: 2, children: 2)", - " Archaebacteria (Bacteria) (used: 2, children: 0)", - " Eubacteria (Bacteria) (used: 1, children: 0)", - ] - # Same with depth=1, which uses a different query internally: - result1 = pretty_format_tags( - self.taxonomy.get_filtered_tags(search_term="bacteria", include_counts=True, depth=1) - ) - assert result1 == [ - "Bacteria (None) (used: 2, children: 2)", - ] - - def test_usage_count_rolls_up_to_ancestors_deep(self) -> None: - """ - When a child tag (depth 3) is applied to an object, it should - roll up the count to all its ancestors when using _get_filtered_tags_deep. - The child tag and each of its ancestors should have usage_count=1. - """ - api.tag_object("obj:1", self.taxonomy, [self.mammalia.value]) - result = pretty_format_tags(self.taxonomy.get_filtered_tags(include_counts=True)) - assert result == [ - "Archaea (None) (used: 0, children: 3)", - " DPANN (Archaea) (used: 0, children: 0)", - " Euryarchaeida (Archaea) (used: 0, children: 0)", - " Proteoarchaeota (Archaea) (used: 0, children: 0)", - "Bacteria (None) (used: 0, children: 2)", - " Archaebacteria (Bacteria) (used: 0, children: 0)", - " Eubacteria (Bacteria) (used: 0, children: 0)", - "Eukaryota (None) (used: 1, children: 5)", - " Animalia (Eukaryota) (used: 1, children: 7)", - " Arthropoda (Animalia) (used: 0, children: 0)", - " Chordata (Animalia) (used: 1, children: 1)", - " Mammalia (Chordata) (used: 1, children: 0)", - " Cnidaria (Animalia) (used: 0, children: 0)", - " Ctenophora (Animalia) (used: 0, children: 0)", - " Gastrotrich (Animalia) (used: 0, children: 0)", - " Placozoa (Animalia) (used: 0, children: 0)", - " Porifera (Animalia) (used: 0, children: 0)", - " Fungi (Eukaryota) (used: 0, children: 0)", - " Monera (Eukaryota) (used: 0, children: 0)", - " Plantae (Eukaryota) (used: 0, children: 0)", - " Protista (Eukaryota) (used: 0, children: 0)", - ] - - def test_usage_count_multiple_objects_same_tag_deep(self) -> None: - """ - When two distinct objects (e.g. separate courses, modules, etc.) are tagged - with the same child tag, it should count 2 for that tag (and roll up 2 - to ancestors). Each distinct object should contribute exactly 1 to the count. - """ - api.tag_object("obj:1", self.taxonomy, [self.chordata.value]) - api.tag_object("obj:2", self.taxonomy, [self.chordata.value]) - result = pretty_format_tags( - self.taxonomy.get_filtered_tags(search_term="chordata", include_counts=True) - ) - assert result == [ - "Eukaryota (None) (used: 2, children: 1)", - " Animalia (Eukaryota) (used: 2, children: 1)", - " Chordata (Animalia) (used: 2, children: 0)", - ] - - def test_usage_count_sibling_tags_same_object_deduplication_deep(self) -> None: - """ - When one object is tagged with two sibling tags (both children of the same - parent), the parent's usage_count should be 1, not 2. It should de-duplicate. - """ - self.taxonomy.allow_multiple = True - self.taxonomy.save() - # Eubacteria and Archaebacteria are both children of Bacteria - api.tag_object("obj:1", self.taxonomy, [self.eubacteria.value, self.archaebacteria.value]) - result = pretty_format_tags( - self.taxonomy.get_filtered_tags(search_term="bacteria", include_counts=True) - ) - assert result == [ - "Bacteria (None) (used: 1, children: 2)", - " Archaebacteria (Bacteria) (used: 1, children: 0)", - " Eubacteria (Bacteria) (used: 1, children: 0)", - ] - - def test_usage_count_sibling_tags_different_objects_deep(self) -> None: - """ - When two different objects are each tagged with a different sibling tag, - the parent's usage_count should be 2, not 1. - """ - api.tag_object("obj:1", self.taxonomy, [self.eubacteria.value]) - api.tag_object("obj:2", self.taxonomy, [self.archaebacteria.value]) - result = pretty_format_tags( - self.taxonomy.get_filtered_tags(search_term="bacteria", include_counts=True) - ) - assert result == [ - "Bacteria (None) (used: 2, children: 2)", - " Archaebacteria (Bacteria) (used: 1, children: 0)", - " Eubacteria (Bacteria) (used: 1, children: 0)", - ] - - def test_usage_count_one_level_root_tags(self) -> None: - """ - _get_filtered_tags_one_level (depth=1) with include_counts=True should - reflect the rolled-up usage count, not just direct usage. - Tagging an object with a child tag should increment the root tag's count. - """ - api.tag_object("obj:1", self.taxonomy, [self.eubacteria.value]) # child of Bacteria - result = pretty_format_tags( - self.taxonomy.get_filtered_tags(depth=1, include_counts=True) - ) - assert result == [ - "Archaea (None) (used: 0, children: 3)", - "Bacteria (None) (used: 1, children: 2)", - "Eukaryota (None) (used: 0, children: 5)", - ] - - def test_usage_count_one_level_child_tags(self) -> None: - """ - When listing children of a tag (depth=1, parent_tag_value=...), the - usage_count of each child should only reflect the objects tagged with - that child or any of its descendants. - """ - api.tag_object("obj:1", self.taxonomy, [self.mammalia.value]) # grandchild of Animalia via Chordata - api.tag_object("obj:2", self.taxonomy, [self.chordata.value]) # direct child of Animalia - result = pretty_format_tags( - self.taxonomy.get_filtered_tags(depth=1, parent_tag_value="Animalia", include_counts=True) - ) - assert result == [ - " Arthropoda (Animalia) (used: 0, children: 0)", - " Chordata (Animalia) (used: 2, children: 1)", - " Cnidaria (Animalia) (used: 0, children: 0)", - " Ctenophora (Animalia) (used: 0, children: 0)", - " Gastrotrich (Animalia) (used: 0, children: 0)", - " Placozoa (Animalia) (used: 0, children: 0)", - " Porifera (Animalia) (used: 0, children: 0)", - ] - - def test_usage_count_three_levels_deep_rollup(self) -> None: - """ - Tagging an object with a depth-3 tag (Chordata) should roll up - to grandparent (Animalia) and great-grandparent (Eukaryota), - verifying the full 3-level lineage query in add_counts_query. - """ - api.tag_object("obj:1", self.taxonomy, [self.animalia.value]) - api.tag_object("obj:1", self.taxonomy, [self.chordata.value]) - result = pretty_format_tags( - self.taxonomy.get_filtered_tags(search_term="chordata", include_counts=True) - ) - assert result == [ - "Eukaryota (None) (used: 1, children: 1)", - " Animalia (Eukaryota) (used: 1, children: 1)", - " Chordata (Animalia) (used: 1, children: 0)", - ] - - def test_usage_count_returns_zero_not_none_deep(self) -> None: - """ - When no object has been tagged with a tag or any of its - descendants, usage_count must be 0 (integer), not None. - """ - result = pretty_format_tags(self.taxonomy.get_filtered_tags(include_counts=True)) - assert result == [ - "Archaea (None) (used: 0, children: 3)", - " DPANN (Archaea) (used: 0, children: 0)", - " Euryarchaeida (Archaea) (used: 0, children: 0)", - " Proteoarchaeota (Archaea) (used: 0, children: 0)", - "Bacteria (None) (used: 0, children: 2)", - " Archaebacteria (Bacteria) (used: 0, children: 0)", - " Eubacteria (Bacteria) (used: 0, children: 0)", - "Eukaryota (None) (used: 0, children: 5)", - " Animalia (Eukaryota) (used: 0, children: 7)", - " Arthropoda (Animalia) (used: 0, children: 0)", - " Chordata (Animalia) (used: 0, children: 1)", - " Mammalia (Chordata) (used: 0, children: 0)", - " Cnidaria (Animalia) (used: 0, children: 0)", - " Ctenophora (Animalia) (used: 0, children: 0)", - " Gastrotrich (Animalia) (used: 0, children: 0)", - " Placozoa (Animalia) (used: 0, children: 0)", - " Porifera (Animalia) (used: 0, children: 0)", - " Fungi (Eukaryota) (used: 0, children: 0)", - " Monera (Eukaryota) (used: 0, children: 0)", - " Plantae (Eukaryota) (used: 0, children: 0)", - " Protista (Eukaryota) (used: 0, children: 0)", - ] - - def test_usage_count_with_search_term_deep(self) -> None: - """ - When using get_filtered_tags() with both a search_term and - include_counts=True, the usage_count returned should still - reflect the true count for each matching tag, not be affected - by the search filter. - """ - api.tag_object("obj:1", self.taxonomy, [self.eubacteria.value]) - api.tag_object("obj:2", self.taxonomy, [self.archaebacteria.value]) - result = pretty_format_tags( - self.taxonomy.get_filtered_tags(search_term="bacteria", include_counts=True) - ) - assert result == [ - "Bacteria (None) (used: 2, children: 2)", - " Archaebacteria (Bacteria) (used: 1, children: 0)", - " Eubacteria (Bacteria) (used: 1, children: 0)", - ] - def test_tree_sort(self) -> None: """ Verify that taxonomies can be sorted correctly in tree orer (case insensitive). @@ -840,20 +598,6 @@ def test_get_filtered_tags(self): {"value": "triple", **common_fields}, ] - def test_get_filtered_tags_with_count(self): - """ - Test basic retrieval of all tags in the taxonomy. - Without counts included. - """ - result = list(self.taxonomy.get_filtered_tags(include_counts=True)) - common_fields = {"child_count": 0, "depth": 0, "parent_value": None, "external_id": None, "_id": None} - assert result == [ - # These should appear in alphabetical order: - {"value": "double", "usage_count": 2, **common_fields}, - {"value": "solo", "usage_count": 1, **common_fields}, - {"value": "triple", "usage_count": 3, **common_fields}, - ] - def test_get_filtered_tags_num_queries(self): """ Test that the number of queries used by get_filtered_tags() is fixed @@ -861,8 +605,6 @@ def test_get_filtered_tags_num_queries(self): """ with self.assertNumQueries(1): self.test_get_filtered_tags() - with self.assertNumQueries(1): - self.test_get_filtered_tags_with_count() def test_get_filtered_tags_with_search(self) -> None: """ @@ -872,8 +614,8 @@ def test_get_filtered_tags_with_search(self) -> None: common_fields = {"child_count": 0, "depth": 0, "parent_value": None, "external_id": None, "_id": None} assert result1 == [ # These should appear in alphabetical order: - {"value": "double", "usage_count": 2, **common_fields}, - {"value": "triple", "usage_count": 3, **common_fields}, + {"value": "double", **common_fields}, + {"value": "triple", **common_fields}, ] # And it should be case insensitive: result2 = list(self.taxonomy.get_filtered_tags(search_term="LE", include_counts=True)) diff --git a/tests/openedx_tagging/test_views.py b/tests/openedx_tagging/test_views.py index 041eec03d..611dc6949 100644 --- a/tests/openedx_tagging/test_views.py +++ b/tests/openedx_tagging/test_views.py @@ -1614,8 +1614,7 @@ def test_large_taxonomy(self): self.client.force_authenticate(user=self.staff) url = self.large_taxonomy_url + "?include_counts" - # 4 queries, including 1 for max depth for counts - with self.assertNumQueries(4): + with self.assertNumQueries(3): response = self.client.get(url) assert response.status_code == status.HTTP_200_OK @@ -2447,6 +2446,425 @@ def test_delete_tag_in_taxonomy_without_subtags(self): existing_tag.refresh_from_db() +class TestTaxonomyTagsUsageCount(TestTaxonomyViewMixin): + """ + Tests the usage_count rollup logic in the taxonomy tags view + """ + + # Taxonomy reference + # + # - Bacteria + # |- Eubacteria + # |- Archaebacteria + # - Archaea + # |- DPANN + # |- Euryarchaeida + # |- Proteoarchaeota + # - Eukaryota (Root) + # |- Animalia (L1) + # | |- Arthropoda + # | |- Chordata (L2) + # | | |- Mammalia (L3) + # | | | |- Carnivora (L4) + # | | | | |- Felidae (L5) + # | | | | | |- Felis (L6) + # | | | |- Canidae + # | |- Cnidaria + # | |- Ctenophora + # | |- Gastrotrich + # | |- Placozoa + # | |- Porifera + # |- Fungi + # |- Monera + # |- Plantae + # |- Protista + + def setUp(self): + super().setUp() + self.taxonomy = Taxonomy.objects.create(name="Usage Count Taxonomy") + self.taxonomy_url = TAXONOMY_TAGS_URL.format(pk=self.taxonomy.pk) + + def test_usage_count_rollup(self): + """ + Test that usage counts correctly roll up from children to parents, + while deduplicating multiple tags on the same object. + """ + # --- Setup Hierarchy --- + # Eukaryota -> Animalia -> (Arthropoda, Chordata, Cnidaria) + eukaryota = Tag.objects.create(taxonomy=self.taxonomy, value="Eukaryota") + animalia = Tag.objects.create(taxonomy=self.taxonomy, value="Animalia", parent=eukaryota) + arthropoda = Tag.objects.create(taxonomy=self.taxonomy, value="Arthropoda", parent=animalia) + chordata = Tag.objects.create(taxonomy=self.taxonomy, value="Chordata", parent=animalia) + cnidaria = Tag.objects.create(taxonomy=self.taxonomy, value="Cnidaria", parent=animalia) + + # --- Setup Tagging --- + # Tags applied as: + # obj1: Arthropoda, Chordata, Cnidaria + # obj2: Arthropoda + obj1_id = "obj1" + obj2_id = "obj2" + + ObjectTag.objects.create(taxonomy=self.taxonomy, tag=arthropoda, object_id=obj1_id) + ObjectTag.objects.create(taxonomy=self.taxonomy, tag=chordata, object_id=obj1_id) + ObjectTag.objects.create(taxonomy=self.taxonomy, tag=cnidaria, object_id=obj1_id) + + ObjectTag.objects.create(taxonomy=self.taxonomy, tag=arthropoda, object_id=obj2_id) + + self.client.force_authenticate(user=self.staff) + + # --- Request all tags with counts --- + response = self.client.get(self.taxonomy_url + "?include_counts&full_depth_threshold=100") + assert response.status_code == status.HTTP_200_OK + + results = {tag["value"]: tag for tag in response.data["results"]} + + # --- Verification --- + # Arthropoda: applied to obj1, obj2 -> count: 2 + assert results["Arthropoda"]["usage_count"] == 2 + + # Chordata: applied to obj1 -> count: 1 + assert results["Chordata"]["usage_count"] == 1 + + # Cnidaria: applied to obj1 -> count: 1 + assert results["Cnidaria"]["usage_count"] == 1 + + # Animalia: applied to obj1 (via Arthropoda, Chordata, Cnidaria) and obj2 (via Arthropoda). + # Should be 2, because it counts '1' per object regardless of how many children are applied. + assert results["Animalia"]["usage_count"] == 2 + + # Eukaryota: same logic as Animalia -> count: 2 + assert results["Eukaryota"]["usage_count"] == 2 + + def test_usage_count_rollup_multi_level(self): + """ + Test that usage counts correctly roll up across more than two levels + of hierarchy. + """ + # --- Setup Hierarchy --- + # Eukaryota -> Animalia -> Chordata -> Mammalia + eukaryota = Tag.objects.create(taxonomy=self.taxonomy, value="Eukaryota") + animalia = Tag.objects.create(taxonomy=self.taxonomy, value="Animalia", parent=eukaryota) + chordata = Tag.objects.create(taxonomy=self.taxonomy, value="Chordata", parent=animalia) + mammalia = Tag.objects.create(taxonomy=self.taxonomy, value="Mammalia", parent=chordata) + + # --- Setup Tagging --- + # obj1: Mammalia + # obj2: Chordata + ObjectTag.objects.create(taxonomy=self.taxonomy, tag=mammalia, object_id="obj1") + ObjectTag.objects.create(taxonomy=self.taxonomy, tag=chordata, object_id="obj2") + + self.client.force_authenticate(user=self.staff) + + # --- Request tags with counts --- + response = self.client.get(self.taxonomy_url + "?include_counts&full_depth_threshold=100") + assert response.status_code == status.HTTP_200_OK + results = {tag["value"]: tag for tag in response.data["results"]} + + # --- Verification --- + # Mammalia: obj1 -> 1 + assert results["Mammalia"]["usage_count"] == 1 + # Chordata: obj1 (via Mammalia), obj2 -> 2 + assert results["Chordata"]["usage_count"] == 2 + # Animalia: obj1 (via Mammalia), obj2 (via Chordata) -> 2 + assert results["Animalia"]["usage_count"] == 2 + # Eukaryota: obj1 (via Mammalia), obj2 (via Chordata) -> 2 + assert results["Eukaryota"]["usage_count"] == 2 + + def test_usage_count_no_rollup_different_objects(self): + """ + Verify that counts are not erroneously shared between different objects + that are tagged with distinct branches of the same hierarchy. + """ + # --- Setup Hierarchy --- + # Eukaryota -> (Animalia, Fungi) + eukaryota = Tag.objects.create(taxonomy=self.taxonomy, value="Eukaryota") + animalia = Tag.objects.create(taxonomy=self.taxonomy, value="Animalia", parent=eukaryota) + fungi = Tag.objects.create(taxonomy=self.taxonomy, value="Fungi", parent=eukaryota) + + # --- Setup Tagging --- + # obj1: Animalia + # obj2: Fungi + ObjectTag.objects.create(taxonomy=self.taxonomy, tag=animalia, object_id="obj1") + ObjectTag.objects.create(taxonomy=self.taxonomy, tag=fungi, object_id="obj2") + + self.client.force_authenticate(user=self.staff) + + # --- Request tags with counts --- + response = self.client.get(self.taxonomy_url + "?include_counts&full_depth_threshold=100") + assert response.status_code == status.HTTP_200_OK + results = {tag["value"]: tag for tag in response.data["results"]} + + # --- Verification --- + assert results["Animalia"]["usage_count"] == 1 + assert results["Fungi"]["usage_count"] == 1 + # Eukaryota should have 2 because it's used on obj1 (via Animalia) and obj2 (via Fungi) + assert results["Eukaryota"]["usage_count"] == 2 + + def test_usage_count_max_depth_rollup(self): + """ + Verify usage_count rollup up to the maximum depth of 7, + ensuring redundant tagging on the same object is deduplicated. + """ + # --- Setup Hierarchy (6 Levels) --- + # Eukaryota -> Animalia -> Chordata -> Mammalia -> Carnivora -> Felidae + eukaryota = Tag.objects.create(taxonomy=self.taxonomy, value="Eukaryota") + animalia = Tag.objects.create(taxonomy=self.taxonomy, value="Animalia", parent=eukaryota) + chordata = Tag.objects.create(taxonomy=self.taxonomy, value="Chordata", parent=animalia) + mammalia = Tag.objects.create(taxonomy=self.taxonomy, value="Mammalia", parent=chordata) + carnivora = Tag.objects.create(taxonomy=self.taxonomy, value="Carnivora", parent=mammalia) + felidae = Tag.objects.create(taxonomy=self.taxonomy, value="Felidae", parent=carnivora) + + # --- Setup Tagging --- + # obj1: Tagged at Felidae AND Carnivora (Redundant tagging) + # Should count as '1' for all tags in its lineage. + ObjectTag.objects.create(taxonomy=self.taxonomy, tag=felidae, object_id="obj1") + ObjectTag.objects.create(taxonomy=self.taxonomy, tag=carnivora, object_id="obj1") + + # obj2: Tagged at Chordata + # Should count as '1' for Chordata, Animalia, and Eukaryota. + ObjectTag.objects.create(taxonomy=self.taxonomy, tag=chordata, object_id="obj2") + + self.client.force_authenticate(user=self.staff) + response = self.client.get(self.taxonomy_url + "?include_counts&full_depth_threshold=7") + assert response.status_code == status.HTTP_200_OK + results = {tag["value"]: tag for tag in response.data["results"]} + + # --- Verification --- + # Felidae: obj1 -> 1 + assert results["Felidae"]["usage_count"] == 1 + # Carnivora: obj1 (twice, but deduplicated) -> 1 + assert results["Carnivora"]["usage_count"] == 1 + # Mammalia: obj1 (via Carnivora/Felidae) -> 1 + assert results["Mammalia"]["usage_count"] == 1 + # Chordata: obj1 (via Mammalia), obj2 -> 2 + assert results["Chordata"]["usage_count"] == 2 + # Animalia: obj1 (via Chordata), obj2 (via Chordata) -> 2 + assert results["Animalia"]["usage_count"] == 2 + # Eukaryota: obj1 (via Animalia), obj2 (via Animalia) -> 2 + assert results["Eukaryota"]["usage_count"] == 2 + + def test_usage_count_one_level_root_and_child_rollup(self): + """ + Verify that usage counts roll up even when querying only a single level. + """ + # Eukaryota -> Animalia -> Chordata + eukaryota = Tag.objects.create(taxonomy=self.taxonomy, value="Eukaryota") + animalia = Tag.objects.create(taxonomy=self.taxonomy, value="Animalia", parent=eukaryota) + chordata = Tag.objects.create(taxonomy=self.taxonomy, value="Chordata", parent=animalia) + + # Tag an object with the deepest tag + ObjectTag.objects.create(taxonomy=self.taxonomy, tag=chordata, object_id="obj1") + + self.client.force_authenticate(user=self.staff) + + # --- Check Root Level (depth=1) --- + # Should show Eukaryota with count 1 + resp_root = self.client.get(self.taxonomy_url + "?include_counts") + results_root = {tag["value"]: tag for tag in resp_root.data["results"]} + assert results_root["Eukaryota"]["usage_count"] == 1 + + def test_usage_count_returns_zero(self): + """ + Ensure usage_count is 0 (int) for unused tags, not None. + """ + Tag.objects.create(taxonomy=self.taxonomy, value="Protista") + + self.client.force_authenticate(user=self.staff) + response = self.client.get(self.taxonomy_url + "?include_counts") + results = {tag["value"]: tag for tag in response.data["results"]} + + assert isinstance(results["Protista"]["usage_count"], int) + assert results["Protista"]["usage_count"] == 0 + + def test_usage_count_with_search_term(self): + """ + Verify usage_count is correct even when the result set is filtered by search. + """ + eukaryota = Tag.objects.create(taxonomy=self.taxonomy, value="Eukaryota") + animalia = Tag.objects.create(taxonomy=self.taxonomy, value="Animalia", parent=eukaryota) + ObjectTag.objects.create(taxonomy=self.taxonomy, tag=animalia, object_id="obj1") + + self.client.force_authenticate(user=self.staff) + + # Search for "Ani" + response = self.client.get(self.taxonomy_url + "?include_counts&search_term=Ani&full_depth_threshold=100") + results = {tag["value"]: tag for tag in response.data["results"]} + + # "Animalia" should match and have count 1 + assert "Animalia" in results + assert results["Animalia"]["usage_count"] == 1 + + def test_usage_count_search_permutations(self): + """ + Extensively test search logic across various depths and match types (partial/complete). + Uses the same search terms and result structure as 'test_api.py' as a handy example. + Ensures usage_count correctly rolls up even when child tags match but parents don't. + """ + # --- Setup Hierarchy (Matching tagging.yaml used in test_api.py) --- + # Bacteria + # - Eubacteria + # - Archaebacteria + # Archaea + # - DPANN + # - Euryarchaeida + # - Proteoarchaeota + # Eukaryota + # - Animalia + # - Arthropoda + # - Chordata + # - Mammalia + # - Cnidaria + # - Ctenophora + # - Gastrotrich + # - Placozoa + # - Porifera + # - Fungi + # - Monera + # - Plantae + # - Protista + + # Roots + bacteria = Tag.objects.create(taxonomy=self.taxonomy, value="Bacteria") + archaea = Tag.objects.create(taxonomy=self.taxonomy, value="Archaea") + eukaryota = Tag.objects.create(taxonomy=self.taxonomy, value="Eukaryota") + + # Bacteria branch + Tag.objects.create(taxonomy=self.taxonomy, value="Eubacteria", parent=bacteria) + archaebacteria = Tag.objects.create(taxonomy=self.taxonomy, value="Archaebacteria", parent=bacteria) + + # Archaea branch + Tag.objects.create(taxonomy=self.taxonomy, value="DPANN", parent=archaea) + euryarchaeida = Tag.objects.create(taxonomy=self.taxonomy, value="Euryarchaeida", parent=archaea) + proteoarchaeota = Tag.objects.create(taxonomy=self.taxonomy, value="Proteoarchaeota", parent=archaea) + + # Eukaryota branch + animalia = Tag.objects.create(taxonomy=self.taxonomy, value="Animalia", parent=eukaryota) + arthropoda = Tag.objects.create(taxonomy=self.taxonomy, value="Arthropoda", parent=animalia) + chordata = Tag.objects.create(taxonomy=self.taxonomy, value="Chordata", parent=animalia) + cnidaria = Tag.objects.create(taxonomy=self.taxonomy, value="Cnidaria", parent=animalia) + Tag.objects.create(taxonomy=self.taxonomy, value="Ctenophora", parent=animalia) + Tag.objects.create(taxonomy=self.taxonomy, value="Gastrotrich", parent=animalia) + Tag.objects.create(taxonomy=self.taxonomy, value="Placozoa", parent=animalia) + Tag.objects.create(taxonomy=self.taxonomy, value="Porifera", parent=animalia) + Tag.objects.create(taxonomy=self.taxonomy, value="Mammalia", parent=chordata) + Tag.objects.create(taxonomy=self.taxonomy, value="Fungi", parent=eukaryota) + Tag.objects.create(taxonomy=self.taxonomy, value="Monera", parent=eukaryota) + plantae = Tag.objects.create(taxonomy=self.taxonomy, value="Plantae", parent=eukaryota) + Tag.objects.create(taxonomy=self.taxonomy, value="Protista", parent=eukaryota) + + # --- Setup Tagging to Exercise usage_counts --- + # Tag a few objects to create counts. + # obj1: Archaebacteria, Arthropoda + ObjectTag.objects.create(taxonomy=self.taxonomy, tag=archaebacteria, object_id="obj1") + ObjectTag.objects.create(taxonomy=self.taxonomy, tag=arthropoda, object_id="obj1") + + # obj2: Euryarchaeida, Cnidaria + ObjectTag.objects.create(taxonomy=self.taxonomy, tag=euryarchaeida, object_id="obj2") + ObjectTag.objects.create(taxonomy=self.taxonomy, tag=cnidaria, object_id="obj2") + + # obj3: Proteoarchaeota, Plantae + ObjectTag.objects.create(taxonomy=self.taxonomy, tag=proteoarchaeota, object_id="obj3") + ObjectTag.objects.create(taxonomy=self.taxonomy, tag=plantae, object_id="obj3") + + self.client.force_authenticate(user=self.staff) + + # SCENARIO 1: search="ChA" + url = self.taxonomy_url + "?include_counts&search_term=ChA&full_depth_threshold=100" + resp = self.client.get(url) + assert resp.status_code == status.HTTP_200_OK + assert pretty_format_tags(resp.data["results"], parent=False) == [ + "Archaea (used: 2, children: 2)", + " Euryarchaeida (used: 1, children: 0)", + " Proteoarchaeota (used: 1, children: 0)", + "Bacteria (used: 1, children: 1)", + " Archaebacteria (used: 1, children: 0)", + ] + + # SCENARIO 2: search="ar" + url = self.taxonomy_url + "?include_counts&search_term=ar&full_depth_threshold=100" + resp = self.client.get(url) + assert resp.status_code == status.HTTP_200_OK + assert pretty_format_tags(resp.data["results"], parent=False) == [ + "Archaea (used: 2, children: 2)", + " Euryarchaeida (used: 1, children: 0)", + " Proteoarchaeota (used: 1, children: 0)", + "Bacteria (used: 1, children: 1)", + " Archaebacteria (used: 1, children: 0)", + "Eukaryota (used: 3, children: 1)", + " Animalia (used: 2, children: 2)", + " Arthropoda (used: 1, children: 0)", + " Cnidaria (used: 1, children: 0)", + ] + + # SCENARIO 3: search="aE" + url = self.taxonomy_url + "?include_counts&search_term=aE&full_depth_threshold=100" + resp = self.client.get(url) + assert resp.status_code == status.HTTP_200_OK + assert pretty_format_tags(resp.data["results"], parent=False) == [ + "Archaea (used: 2, children: 2)", + " Euryarchaeida (used: 1, children: 0)", + " Proteoarchaeota (used: 1, children: 0)", + "Bacteria (used: 1, children: 1)", + " Archaebacteria (used: 1, children: 0)", + "Eukaryota (used: 3, children: 1)", + " Plantae (used: 1, children: 0)", + ] + + # SCENARIO 4: search="a" + url = self.taxonomy_url + "?include_counts&search_term=a&full_depth_threshold=100" + resp = self.client.get(url) + assert resp.status_code == status.HTTP_200_OK + assert pretty_format_tags(resp.data["results"], parent=False) == [ + "Archaea (used: 2, children: 3)", + " DPANN (used: 0, children: 0)", + " Euryarchaeida (used: 1, children: 0)", + " Proteoarchaeota (used: 1, children: 0)", + "Bacteria (used: 1, children: 2)", + " Archaebacteria (used: 1, children: 0)", + " Eubacteria (used: 0, children: 0)", + "Eukaryota (used: 3, children: 4)", + " Animalia (used: 2, children: 7)", + " Arthropoda (used: 1, children: 0)", + " Chordata (used: 0, children: 1)", + " Mammalia (used: 0, children: 0)", + " Cnidaria (used: 1, children: 0)", + " Ctenophora (used: 0, children: 0)", + " Gastrotrich (used: 0, children: 0)", + " Placozoa (used: 0, children: 0)", + " Porifera (used: 0, children: 0)", + " Monera (used: 0, children: 0)", + " Plantae (used: 1, children: 0)", + " Protista (used: 0, children: 0)", + ] + + def test_usage_count_sibling_and_ancestor_deduplication(self): + """ + Test deduplication when multiple children of the same parent are applied to the same object. + """ + animalia = Tag.objects.create(taxonomy=self.taxonomy, value="Animalia") + arthropoda = Tag.objects.create(taxonomy=self.taxonomy, value="Arthropoda", parent=animalia) + chordata = Tag.objects.create(taxonomy=self.taxonomy, value="Chordata", parent=animalia) + + # obj1: tagged with both siblings + ObjectTag.objects.create(taxonomy=self.taxonomy, tag=arthropoda, object_id="obj1") + ObjectTag.objects.create(taxonomy=self.taxonomy, tag=chordata, object_id="obj1") + + # obj2: tagged with only one sibling + ObjectTag.objects.create(taxonomy=self.taxonomy, tag=arthropoda, object_id="obj2") + + self.client.force_authenticate(user=self.staff) + response = self.client.get(self.taxonomy_url + "?include_counts&full_depth_threshold=100") + results = {tag["value"]: tag for tag in response.data["results"]} + + # Arthropoda: obj1, obj2 -> 2 + assert results["Arthropoda"]["usage_count"] == 2 + # Chordata: obj1 -> 1 + assert results["Chordata"]["usage_count"] == 1 + # Animalia: obj1 (via Arthropoda/Chordata), obj2 (via Arthropoda) -> 2 + # Deduplication check: obj1 only counts as 1 for Animalia even though it has both Arthropoda and Chordata. + assert results["Animalia"]["usage_count"] == 2 + + class ImportTaxonomyMixin(TestTaxonomyViewMixin): """ Mixin to test importing taxonomies. From 155d03b574f74ad93c729cb2bd564730010fe2d1 Mon Sep 17 00:00:00 2001 From: tbain Date: Mon, 13 Apr 2026 10:26:51 -0700 Subject: [PATCH 16/17] feat: #253 Addressing code review comments --- src/openedx_core/__init__.py | 2 +- src/openedx_tagging/api.py | 51 ++++++++++++++++++++++-- src/openedx_tagging/models/base.py | 8 +--- src/openedx_tagging/rest_api/v1/views.py | 44 +++++--------------- tests/openedx_tagging/test_api.py | 2 +- tests/openedx_tagging/test_models.py | 11 +++-- tests/openedx_tagging/test_views.py | 39 +++++++++++------- 7 files changed, 90 insertions(+), 67 deletions(-) diff --git a/src/openedx_core/__init__.py b/src/openedx_core/__init__.py index d5c904422..0c2d2a835 100644 --- a/src/openedx_core/__init__.py +++ b/src/openedx_core/__init__.py @@ -6,4 +6,4 @@ """ # The version for the entire repository -__version__ = "0.38.2" +__version__ = "0.38.3" diff --git a/src/openedx_tagging/api.py b/src/openedx_tagging/api.py index 0998a6ab5..352ce7737 100644 --- a/src/openedx_tagging/api.py +++ b/src/openedx_tagging/api.py @@ -12,7 +12,8 @@ """ from __future__ import annotations -from typing import Any +from collections import defaultdict +from typing import Any, Counter from django.db import models, transaction from django.db.models import F, QuerySet, Value @@ -116,7 +117,6 @@ def search_tags( taxonomy: Taxonomy, search_term: str, exclude_object_id: str | None = None, - include_counts: bool = False, ) -> TagDataQuerySet: """ Returns a list of all tags that contains `search_term` of the given @@ -138,7 +138,6 @@ def search_tags( qs = taxonomy.cast().get_filtered_tags( search_term=search_term, excluded_values=excluded_values, - include_counts=include_counts, ) return qs @@ -525,3 +524,49 @@ def unmark_copied_tags(object_id: str) -> None: Update copied object tags on the given object to mark them as "not copied". """ ObjectTag.objects.filter(object_id=object_id).update(is_copied=False) + + +def add_usage_counts(taxonomy: Taxonomy, tag_data: TagDataQuerySet) -> TagDataQuerySet: + """ + Add usage counts to the query result. + + Not a simple raw count of each tags uasge. A tag can be directly + applied to an object, which can be a course, library, module, + or something else. + + A tag can also be indirectly applied when some of its children + are applied to an object, it is considered automatically applied. + So, if the tags "Chemistry" and "Physics" are applied once + each to different objects, their parent tag "Natural Science" is + considered indirectly applied to 2 objects. + + Deduplication: A tag can only be applied to a single object once. + So if two child tags are applied to the same object, e.g. + "Chemistry" and "Physics" are applied to the same course, the + parent tag, "Natural Science" is only applied to it once, + because no tag can be applied to the same object twice. + + For performance reasons, we call this function with the list result of the + QuerySet so we can then add the counts in-memory rather than annotate to a + QuerySet which would require a very expensive annotation to join the + in-memory data to the original QuerySet. + """ + + object_tags = taxonomy.objecttag_set.values_list("object_id", "tag__lineage") + tag_counts: Counter[str] = Counter() + object_tag_lineage_seen: defaultdict[str, set] = defaultdict(set) + + for object_id, tag_lineage in object_tags: + # split the lineages to get a dict of {tag.value: [lineages]} + lineage_tags = list(tag_lineage.split('\t')) if tag_lineage else [] + # de-duplicate based on if the lineage is already 'seen' per object + unseen_tags = [t for t in lineage_tags if t not in object_tag_lineage_seen[object_id]] + + tag_counts.update(unseen_tags) + object_tag_lineage_seen[object_id].update(unseen_tags) + + # In-memory 'annotation'; this is faster than using annotate() on the QuerySet. + for row in tag_data: + row["usage_count"] = tag_counts.get(row["value"], 0) + + return tag_data diff --git a/src/openedx_tagging/models/base.py b/src/openedx_tagging/models/base.py index 046219ed3..000d4d338 100644 --- a/src/openedx_tagging/models/base.py +++ b/src/openedx_tagging/models/base.py @@ -426,7 +426,6 @@ def get_filtered_tags( # pylint: disable=too-many-positional-arguments depth: int | None = None, parent_tag_value: str | None = None, search_term: str | None = None, - include_counts: bool = False, excluded_values: list[str] | None = None, ) -> TagDataQuerySet: """ @@ -451,7 +450,7 @@ def get_filtered_tags( # pylint: disable=too-many-positional-arguments if self.allow_free_text: if parent_tag_value is not None: raise ValueError("Cannot specify a parent tag ID for free text taxonomies") - result = self._get_filtered_tags_free_text(search_term=search_term, include_counts=include_counts) + result = self._get_filtered_tags_free_text(search_term=search_term) if excluded_values: return result.exclude(value__in=excluded_values) else: @@ -460,7 +459,6 @@ def get_filtered_tags( # pylint: disable=too-many-positional-arguments result = self._get_filtered_tags_one_level( parent_tag_value=parent_tag_value, search_term=search_term, - include_counts=include_counts, ) if excluded_values: return result.exclude(value__in=excluded_values) @@ -470,7 +468,6 @@ def get_filtered_tags( # pylint: disable=too-many-positional-arguments return self._get_filtered_tags_deep( parent_tag_value=parent_tag_value, search_term=search_term, - include_counts=include_counts, excluded_values=excluded_values, ) else: @@ -479,7 +476,6 @@ def get_filtered_tags( # pylint: disable=too-many-positional-arguments def _get_filtered_tags_free_text( self, search_term: str | None, - include_counts: bool, # pylint: disable=unused-argument ) -> TagDataQuerySet: """ Implementation of get_filtered_tags() for free text taxonomies. @@ -506,7 +502,6 @@ def _get_filtered_tags_one_level( self, parent_tag_value: str | None, search_term: str | None, - include_counts: bool, # pylint: disable=unused-argument ) -> TagDataQuerySet: """ Implementation of get_filtered_tags() for closed taxonomies, where @@ -536,7 +531,6 @@ def _get_filtered_tags_deep( self, parent_tag_value: str | None, search_term: str | None, - include_counts: bool, # pylint: disable=unused-argument excluded_values: list[str] | None, ) -> TagDataQuerySet: """ diff --git a/src/openedx_tagging/rest_api/v1/views.py b/src/openedx_tagging/rest_api/v1/views.py index 237bd890b..4c37784a3 100644 --- a/src/openedx_tagging/rest_api/v1/views.py +++ b/src/openedx_tagging/rest_api/v1/views.py @@ -3,8 +3,6 @@ """ from __future__ import annotations -from collections import Counter, defaultdict - from django.core import exceptions from django.db import models from django.http import Http404, HttpResponse @@ -19,6 +17,7 @@ from ...api import ( TagDoesNotExist, add_tag_to_taxonomy, + add_usage_counts, create_taxonomy, delete_tags_from_taxonomy, get_object_tag_counts, @@ -845,18 +844,21 @@ def get_queryset(self) -> TagDataQuerySet: parent_tag_value=parent_tag_value, search_term=search_term, depth=depth, - include_counts=include_counts, ) if depth == 1: # We're already returning just a single level. It will be paginated normally. if include_counts: - return self._add_counts(results) + results_with_counts = add_usage_counts(self.get_taxonomy(), results) + return results_with_counts + return results elif full_depth_threshold and len(results) < full_depth_threshold: # We can load and display all the tags in this (sub)tree at once: self.pagination_class = DisabledTagsPagination if include_counts: - return self._add_counts(results) + results_with_counts = add_usage_counts(self.get_taxonomy(), results) + return results_with_counts + return results else: # We had to do a deep query, but we will only return one level of results. @@ -865,39 +867,11 @@ def get_queryset(self) -> TagDataQuerySet: # It will be paginated normally. filtered_results = results.filter(parent_value=parent_tag_value) if include_counts: - return self._add_counts(filtered_results) + results_with_counts = add_usage_counts(self.get_taxonomy(), results) + return results_with_counts return filtered_results - def _add_counts(self, tag_data: TagDataQuerySet) -> TagDataQuerySet: - """ - Add usage counts to a list of tag data dictionaries. For performance - reasons, we call this function with the list result of the - QuerySet so we can then add the counts in-memory rather than to a - QuerySet which would require a very expensive annotation to join the - in-memory data to the original QuerySet. - """ - - taxonomy = self.get_taxonomy() - object_tags = taxonomy.objecttag_set.values_list("object_id", "tag__lineage") - tag_counts: Counter[str] = Counter() - object_tag_lineage_seen: defaultdict[str, set] = defaultdict(set) - - for object_id, tag_lineage in object_tags: - # split the lineages to get a dict of {tag.value: [lineages]} - lineage_tags = list(tag_lineage.split('\t')) if tag_lineage else [] - # de-duplicate based on if the lineage is already 'seen' per object - unseen_tags = [t for t in lineage_tags if t not in object_tag_lineage_seen[object_id]] - - tag_counts.update(unseen_tags) - object_tag_lineage_seen[object_id].update(unseen_tags) - - # In-memory 'annotation'; this is faster than using annotate() on the QuerySet. - for row in tag_data: - row["usage_count"] = tag_counts.get(row["value"], 0) - - return tag_data - def post(self, request, *args, **kwargs): """ Creates new Tag in Taxonomy and returns the newly created Tag. diff --git a/tests/openedx_tagging/test_api.py b/tests/openedx_tagging/test_api.py index d3accf4b4..83ed1010b 100644 --- a/tests/openedx_tagging/test_api.py +++ b/tests/openedx_tagging/test_api.py @@ -817,7 +817,7 @@ def test_autocomplete_tags_closed(self, search: str, expected: list[str]) -> Non _value=value, ).save() - result = tagging_api.search_tags(closed_taxonomy, search, include_counts=True) + result = tagging_api.search_tags(closed_taxonomy, search) assert pretty_format_tags(result, parent=False) == expected def test_autocomplete_tags_closed_omit_object(self) -> None: diff --git a/tests/openedx_tagging/test_models.py b/tests/openedx_tagging/test_models.py index 22d60d430..1808316a7 100644 --- a/tests/openedx_tagging/test_models.py +++ b/tests/openedx_tagging/test_models.py @@ -321,9 +321,9 @@ class TestFilteredTagsClosedTaxonomy(TestTagTaxonomyMixin, TestCase): def test_get_root(self) -> None: """ Test basic retrieval of root tags in the closed taxonomy, using - get_filtered_tags(). Without counts included. + get_filtered_tags(). """ - result = list(self.taxonomy.get_filtered_tags(depth=1, include_counts=False)) + result = list(self.taxonomy.get_filtered_tags(depth=1)) common_fields = {"depth": 0, "parent_value": None, "external_id": None} for r in result: del r["_id"] # Remove the internal database IDs; they aren't interesting here and a other tests check them @@ -587,9 +587,8 @@ def setUp(self): def test_get_filtered_tags(self): """ Test basic retrieval of all tags in the taxonomy. - Without counts included. """ - result = list(self.taxonomy.get_filtered_tags(include_counts=False)) + result = list(self.taxonomy.get_filtered_tags()) common_fields = {"child_count": 0, "depth": 0, "parent_value": None, "external_id": None, "_id": None} assert result == [ # These should appear in alphabetical order: @@ -610,7 +609,7 @@ def test_get_filtered_tags_with_search(self) -> None: """ Test basic retrieval of only matching tags. """ - result1 = list(self.taxonomy.get_filtered_tags(search_term="le", include_counts=True)) + result1 = list(self.taxonomy.get_filtered_tags(search_term="le")) common_fields = {"child_count": 0, "depth": 0, "parent_value": None, "external_id": None, "_id": None} assert result1 == [ # These should appear in alphabetical order: @@ -618,7 +617,7 @@ def test_get_filtered_tags_with_search(self) -> None: {"value": "triple", **common_fields}, ] # And it should be case insensitive: - result2 = list(self.taxonomy.get_filtered_tags(search_term="LE", include_counts=True)) + result2 = list(self.taxonomy.get_filtered_tags(search_term="LE")) assert result1 == result2 diff --git a/tests/openedx_tagging/test_views.py b/tests/openedx_tagging/test_views.py index 611dc6949..e9c97a6a8 100644 --- a/tests/openedx_tagging/test_views.py +++ b/tests/openedx_tagging/test_views.py @@ -2448,10 +2448,12 @@ def test_delete_tag_in_taxonomy_without_subtags(self): class TestTaxonomyTagsUsageCount(TestTaxonomyViewMixin): """ - Tests the usage_count rollup logic in the taxonomy tags view + Tests the usage count of tags in a taxonomy, verifies that the + usage count is correct according to the rules as described in the + comments in src/openedx_tagging/api.py:add_usage_counts() """ - # Taxonomy reference + # Taxonomy reference as used in tests below # # - Bacteria # |- Eubacteria @@ -2484,10 +2486,15 @@ def setUp(self): self.taxonomy = Taxonomy.objects.create(name="Usage Count Taxonomy") self.taxonomy_url = TAXONOMY_TAGS_URL.format(pk=self.taxonomy.pk) - def test_usage_count_rollup(self): + def test_simple_usage_count_with_lineage_and_deduplication(self): """ - Test that usage counts correctly roll up from children to parents, - while deduplicating multiple tags on the same object. + Test that usage counts correctly 'roll up' from children to parents, + while deduplicating multiple tags applied to the same object. + + This test is a basic case to verify that the tags are correctly + counted according to business rules and deduplication + requirements; Animalia and Eukaryota should not be counted + more than once per object, the children should be counted once each. """ # --- Setup Hierarchy --- # Eukaryota -> Animalia -> (Arthropoda, Chordata, Cnidaria) @@ -2535,10 +2542,13 @@ def test_usage_count_rollup(self): # Eukaryota: same logic as Animalia -> count: 2 assert results["Eukaryota"]["usage_count"] == 2 - def test_usage_count_rollup_multi_level(self): + def test_usage_count_through_multiple_levels(self): """ - Test that usage counts correctly roll up across more than two levels - of hierarchy. + Test that usage count is correctly calculated across multiple levels. + Apply a simple set of tags to some objects and verify that the + usage_counts are correctly calculated, verifying that the ancestor + tags are correctly applied and de-deuplicated across the entire depth + of the taxonomy """ # --- Setup Hierarchy --- # Eukaryota -> Animalia -> Chordata -> Mammalia @@ -2570,7 +2580,7 @@ def test_usage_count_rollup_multi_level(self): # Eukaryota: obj1 (via Mammalia), obj2 (via Chordata) -> 2 assert results["Eukaryota"]["usage_count"] == 2 - def test_usage_count_no_rollup_different_objects(self): + def test_usage_count_across_different_objects(self): """ Verify that counts are not erroneously shared between different objects that are tagged with distinct branches of the same hierarchy. @@ -2600,10 +2610,10 @@ def test_usage_count_no_rollup_different_objects(self): # Eukaryota should have 2 because it's used on obj1 (via Animalia) and obj2 (via Fungi) assert results["Eukaryota"]["usage_count"] == 2 - def test_usage_count_max_depth_rollup(self): + def test_usage_count_max_depth(self): """ - Verify usage_count rollup up to the maximum depth of 7, - ensuring redundant tagging on the same object is deduplicated. + Verify usage_count up to the maximum depth of 7, ensuring redundant + tagging on the same object is deduplicated. """ # --- Setup Hierarchy (6 Levels) --- # Eukaryota -> Animalia -> Chordata -> Mammalia -> Carnivora -> Felidae @@ -2643,9 +2653,10 @@ def test_usage_count_max_depth_rollup(self): # Eukaryota: obj1 (via Animalia), obj2 (via Animalia) -> 2 assert results["Eukaryota"]["usage_count"] == 2 - def test_usage_count_one_level_root_and_child_rollup(self): + def test_usage_count_only_at_root_when_child_applied(self): """ - Verify that usage counts roll up even when querying only a single level. + Verify that usage_count for a tag is correct, even if we only query for + the root level tag and is only used indirectly because a child is applied. """ # Eukaryota -> Animalia -> Chordata eukaryota = Tag.objects.create(taxonomy=self.taxonomy, value="Eukaryota") From 0880c20bc78754f493fddbfb276e749a1306b200 Mon Sep 17 00:00:00 2001 From: tbain Date: Mon, 13 Apr 2026 12:41:38 -0700 Subject: [PATCH 17/17] feat: #253 Addressing code review comments --- src/openedx_core/__init__.py | 2 +- src/openedx_tagging/api.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/openedx_core/__init__.py b/src/openedx_core/__init__.py index 01d5f52bf..05861982c 100644 --- a/src/openedx_core/__init__.py +++ b/src/openedx_core/__init__.py @@ -6,4 +6,4 @@ """ # The version for the entire repository -__version__ = "0.39.1" +__version__ = "0.39.2" diff --git a/src/openedx_tagging/api.py b/src/openedx_tagging/api.py index 352ce7737..5763cb7df 100644 --- a/src/openedx_tagging/api.py +++ b/src/openedx_tagging/api.py @@ -530,7 +530,7 @@ def add_usage_counts(taxonomy: Taxonomy, tag_data: TagDataQuerySet) -> TagDataQu """ Add usage counts to the query result. - Not a simple raw count of each tags uasge. A tag can be directly + Not a simple raw count of each tags usage. A tag can be directly applied to an object, which can be a course, library, module, or something else.