From 4fb8da48b3184e8feecae3903562de6ebccf2956 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Wed, 18 Mar 2026 15:43:29 -0400 Subject: [PATCH 1/6] fix: return correct taxonomy depth up to max depth --- src/openedx_tagging/models/base.py | 26 +++++++++++++++++++++----- tests/openedx_tagging/test_models.py | 26 +++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/src/openedx_tagging/models/base.py b/src/openedx_tagging/models/base.py index 668ba73e..e6ef0bfb 100644 --- a/src/openedx_tagging/models/base.py +++ b/src/openedx_tagging/models/base.py @@ -530,12 +530,28 @@ def _get_filtered_tags_deep( else: main_parent_id = None + qs: models.QuerySet = self.tag_set.none() # empty queryset assert TAXONOMY_MAX_DEPTH == 3 # If we change TAXONOMY_MAX_DEPTH we need to change this query code: - qs: models.QuerySet = self.tag_set.filter( - Q(parent_id=main_parent_id) | - Q(parent__parent_id=main_parent_id) | - Q(parent__parent__parent_id=main_parent_id) - ) + + if (main_parent_id is not None): + # If we're starting from a specific parent tag, we only want to include tags that are descendants of that parent tag. + qs: models.QuerySet = self.tag_set.filter( + # Main tag at depth 0is not included in the results + Q(parent_id=main_parent_id) | # Direct children of the parent tag at depth 1 + Q(parent__parent_id=main_parent_id) | # Grandchildren of the parent tag at depth 2 + Q(parent__parent__parent_id=main_parent_id) # Great-grandchildren of the parent tag at depth 3 + ) + else: + # If we're starting from the top, we want to include all tags up to the max depth. + qs: models.QuerySet = self.tag_set.filter( + Q(parent=None) | # Root tags at depth 0 + Q(parent__parent=None) | # Children of root tags at depth 1 + Q(parent__parent__parent=None) | # Grandchildren of root tags at depth 2 + Q(parent__parent__parent__parent=None) # Great-grandchildren of root tags at depth 3 + ) + + # Regardless of where we start, we only support tags up to TAXONOMY_MAX_DEPTH from the root. + qs = qs.filter(parent__parent__parent__parent=None) if search_term: # We need to do an additional query to find all the tags that match the search term, then limit the diff --git a/tests/openedx_tagging/test_models.py b/tests/openedx_tagging/test_models.py index 67ae68e2..e6b6610b 100644 --- a/tests/openedx_tagging/test_models.py +++ b/tests/openedx_tagging/test_models.py @@ -443,7 +443,8 @@ def test_get_all(self) -> None: "Eukaryota (None) (children: 5 + 8)", " Animalia (Eukaryota) (children: 7 + 1)", " Arthropoda (Animalia) (children: 0)", - " Chordata (Animalia) (children: 1)", # note this has a child but the child is not included + " Chordata (Animalia) (children: 1)", + " Mammalia (Chordata) (children: 0)", " Cnidaria (Animalia) (children: 0)", " Ctenophora (Animalia) (children: 0)", " Gastrotrich (Animalia) (children: 0)", @@ -622,8 +623,31 @@ def test_descendant_counts(self) -> None: "Interests (None) (used: 0, children: 1 + 7)", " Holland Codes (Interests) (used: 0, children: 1 + 6)", " Interests - Holland Codes (Holland Codes) (used: 0, children: 6)", + " Artistic (Interests - Holland Codes) (used: 0, children: 0)", + " Conventional (Interests - Holland Codes) (used: 0, children: 0)", + " Enterprising (Interests - Holland Codes) (used: 0, children: 0)", + " Investigative (Interests - Holland Codes) (used: 0, children: 0)", + " Realistic (Interests - Holland Codes) (used: 0, children: 0)", + " Social (Interests - Holland Codes) (used: 0, children: 0)", ] + def test_parent_query_depth_limit(self) -> None: + """ + Verify that filtering below a specific parent still respects the + taxonomy max supported depth from the root (depth <= 3). + """ + taxonomy = api.create_taxonomy("Deep Parent Filter") + root = api.add_tag_to_taxonomy(taxonomy, "Root") + depth1 = api.add_tag_to_taxonomy(taxonomy, "Depth1", parent_tag_value=root.value) + depth2 = api.add_tag_to_taxonomy(taxonomy, "Depth2", parent_tag_value=depth1.value) + depth3 = api.add_tag_to_taxonomy(taxonomy, "Depth3", parent_tag_value=depth2.value) + api.add_tag_to_taxonomy(taxonomy, "Depth4", parent_tag_value=depth3.value) + + result = list(taxonomy.get_filtered_tags(depth=None, parent_tag_value=depth1.value)) + result_values = [row["value"] for row in result] + + assert result_values == ["Depth2", "Depth3"] + class TestFilteredTagsFreeTextTaxonomy(TestCase): """ From 40265676e325fe8adb64749e9ed4667287ade839 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Thu, 19 Mar 2026 19:17:28 -0400 Subject: [PATCH 2/6] refactor: make query readable --- src/openedx_tagging/models/base.py | 50 ++++++++++++++++-------------- 1 file changed, 27 insertions(+), 23 deletions(-) diff --git a/src/openedx_tagging/models/base.py b/src/openedx_tagging/models/base.py index e6ef0bfb..4ed3b217 100644 --- a/src/openedx_tagging/models/base.py +++ b/src/openedx_tagging/models/base.py @@ -513,6 +513,26 @@ def _get_filtered_tags_one_level( qs = qs.annotate(usage_count=models.Subquery(obj_tags.values('count'))) return qs # type: ignore[return-value] + def _filter_by_descendant_tags(self, tag_id: int, qs: models.QuerySet) -> models.QuerySet: + """ + Helper function to get all descendant tags of a given parent tag + up to the supported maximum depth. + """ + children = Q(parent_id=tag_id) + grandchildren = Q(parent__parent_id=tag_id) + great_grandchildren = Q(parent__parent__parent_id=tag_id) + return qs.filter( + children | + grandchildren | + great_grandchildren + ) + + def _filter_by_max_depth(self, qs: models.QuerySet) -> models.QuerySet: + """ + Helper function to get all tags up to the supported maximum depth. + """ + return qs.filter(Q(parent__parent__parent__parent_id=None)) + def _get_filtered_tags_deep( self, parent_tag_value: str | None, @@ -525,33 +545,17 @@ def _get_filtered_tags_deep( we're including tags from multiple levels of the hierarchy. """ # All tags (possibly below a certain tag?) in the closed taxonomy, up to depth TAXONOMY_MAX_DEPTH - if parent_tag_value: - main_parent_id = self.tag_for_value(parent_tag_value).pk - else: - main_parent_id = None + main_parent_id = self.tag_for_value(parent_tag_value).pk if parent_tag_value else None + + qs: models.QuerySet = self.tag_set - qs: models.QuerySet = self.tag_set.none() # empty queryset assert TAXONOMY_MAX_DEPTH == 3 # If we change TAXONOMY_MAX_DEPTH we need to change this query code: - if (main_parent_id is not None): - # If we're starting from a specific parent tag, we only want to include tags that are descendants of that parent tag. - qs: models.QuerySet = self.tag_set.filter( - # Main tag at depth 0is not included in the results - Q(parent_id=main_parent_id) | # Direct children of the parent tag at depth 1 - Q(parent__parent_id=main_parent_id) | # Grandchildren of the parent tag at depth 2 - Q(parent__parent__parent_id=main_parent_id) # Great-grandchildren of the parent tag at depth 3 - ) - else: - # If we're starting from the top, we want to include all tags up to the max depth. - qs: models.QuerySet = self.tag_set.filter( - Q(parent=None) | # Root tags at depth 0 - Q(parent__parent=None) | # Children of root tags at depth 1 - Q(parent__parent__parent=None) | # Grandchildren of root tags at depth 2 - Q(parent__parent__parent__parent=None) # Great-grandchildren of root tags at depth 3 - ) + # If a main parent is specified, we only want to include its descendants, otherwise, all tags. + if main_parent_id is not None: + qs = self._filter_by_descendant_tags(tag_id=main_parent_id, qs=qs) - # Regardless of where we start, we only support tags up to TAXONOMY_MAX_DEPTH from the root. - qs = qs.filter(parent__parent__parent__parent=None) + qs = self._filter_by_max_depth(qs) if search_term: # We need to do an additional query to find all the tags that match the search term, then limit the From b58ba70251635086698c9776b5a7497a0de29b27 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Thu, 19 Mar 2026 19:30:50 -0400 Subject: [PATCH 3/6] fix: lint --- src/openedx_tagging/models/base.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/openedx_tagging/models/base.py b/src/openedx_tagging/models/base.py index 4ed3b217..41062a7b 100644 --- a/src/openedx_tagging/models/base.py +++ b/src/openedx_tagging/models/base.py @@ -513,7 +513,7 @@ def _get_filtered_tags_one_level( qs = qs.annotate(usage_count=models.Subquery(obj_tags.values('count'))) return qs # type: ignore[return-value] - def _filter_by_descendant_tags(self, tag_id: int, qs: models.QuerySet) -> models.QuerySet: + def _descendants_filter(self, tag_id: int) -> Q: """ Helper function to get all descendant tags of a given parent tag up to the supported maximum depth. @@ -521,17 +521,17 @@ def _filter_by_descendant_tags(self, tag_id: int, qs: models.QuerySet) -> models children = Q(parent_id=tag_id) grandchildren = Q(parent__parent_id=tag_id) great_grandchildren = Q(parent__parent__parent_id=tag_id) - return qs.filter( + return ( children | grandchildren | great_grandchildren ) - def _filter_by_max_depth(self, qs: models.QuerySet) -> models.QuerySet: + def _max_depth_filter(self) -> Q: """ Helper function to get all tags up to the supported maximum depth. """ - return qs.filter(Q(parent__parent__parent__parent_id=None)) + return Q(parent__parent__parent__parent_id=None) def _get_filtered_tags_deep( self, @@ -547,15 +547,13 @@ def _get_filtered_tags_deep( # All tags (possibly below a certain tag?) in the closed taxonomy, up to depth TAXONOMY_MAX_DEPTH main_parent_id = self.tag_for_value(parent_tag_value).pk if parent_tag_value else None - qs: models.QuerySet = self.tag_set - assert TAXONOMY_MAX_DEPTH == 3 # If we change TAXONOMY_MAX_DEPTH we need to change this query code: + qs = self.tag_set.filter(self._max_depth_filter()) + # If a main parent is specified, we only want to include its descendants, otherwise, all tags. if main_parent_id is not None: - qs = self._filter_by_descendant_tags(tag_id=main_parent_id, qs=qs) - - qs = self._filter_by_max_depth(qs) + qs = qs.filter(self._descendants_filter(tag_id=main_parent_id)) if search_term: # We need to do an additional query to find all the tags that match the search term, then limit the From 8605ab7fd419ee2074e4f95071dd2ada17b43603 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Thu, 19 Mar 2026 19:41:31 -0400 Subject: [PATCH 4/6] fix: types --- src/openedx_tagging/models/base.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/openedx_tagging/models/base.py b/src/openedx_tagging/models/base.py index 41062a7b..95be018e 100644 --- a/src/openedx_tagging/models/base.py +++ b/src/openedx_tagging/models/base.py @@ -549,7 +549,7 @@ def _get_filtered_tags_deep( assert TAXONOMY_MAX_DEPTH == 3 # If we change TAXONOMY_MAX_DEPTH we need to change this query code: - qs = self.tag_set.filter(self._max_depth_filter()) + qs: models.QuerySet = self.tag_set.filter(self._max_depth_filter()) # If a main parent is specified, we only want to include its descendants, otherwise, all tags. if main_parent_id is not None: @@ -569,7 +569,7 @@ def _get_filtered_tags_deep( if pk is not None: matching_ids.append(pk) qs = qs.filter(pk__in=matching_ids) - qs = qs.annotate( + qs = qs.annotate( # type: ignore[no-redef] child_count=models.Count("children", filter=Q(children__pk__in=matching_ids), distinct=True), grandchild_count=models.Count( "children__children", filter=Q(children__children__pk__in=matching_ids), distinct=True, @@ -579,17 +579,21 @@ def _get_filtered_tags_deep( filter=Q(children__children__children__pk__in=matching_ids), ), ) - qs = qs.annotate(descendant_count=F("child_count") + F("grandchild_count") + F("great_grandchild_count")) + qs = qs.annotate( # type: ignore[no-redef] + descendant_count=F("child_count") + F("grandchild_count") + F("great_grandchild_count") + ) elif excluded_values: raise NotImplementedError("Using excluded_values without search_term is not currently supported.") # We could implement this in the future but I'd prefer to get rid of the "excluded_values" API altogether. # It remains to be seen if it's useful to do that on the backend, or if we can do it better/simpler on the # frontend. else: - qs = qs.annotate(child_count=models.Count("children", distinct=True)) + qs = qs.annotate(child_count=models.Count("children", distinct=True)) # type: ignore[no-redef] qs = qs.annotate(grandchild_count=models.Count("children__children", distinct=True)) qs = qs.annotate(great_grandchild_count=models.Count("children__children__children")) - qs = qs.annotate(descendant_count=F("child_count") + F("grandchild_count") + F("great_grandchild_count")) + qs = qs.annotate( # type: ignore[no-redef] + descendant_count=F("child_count") + F("grandchild_count") + F("great_grandchild_count") + ) # Add the "depth" to each tag: qs = Tag.annotate_depth(qs) From 3527655cd00a074a021799c58c7ed6cf638b226f Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Thu, 19 Mar 2026 20:30:27 -0400 Subject: [PATCH 5/6] fix: tests --- src/openedx_tagging/models/base.py | 3 ++- tests/openedx_tagging/import_export/test_template.py | 5 ++--- tests/openedx_tagging/test_api.py | 12 +++++++----- tests/openedx_tagging/test_views.py | 1 + 4 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/openedx_tagging/models/base.py b/src/openedx_tagging/models/base.py index 95be018e..27a8592d 100644 --- a/src/openedx_tagging/models/base.py +++ b/src/openedx_tagging/models/base.py @@ -150,7 +150,8 @@ def depth(self) -> int: def annotate_depth(qs: models.QuerySet) -> models.QuerySet: """ Given a query that loads Tag objects, annotate it with the depth of - each tag. + each tag. Depth is 0-indexed: root tags have depth=0, their children have depth=1, and so on. + That means that a taxonomy consisting of 4 levels have depths 0, 1, 2, and 3. """ return qs.annotate(depth=models.Case( models.When(parent_id=None, then=0), diff --git a/tests/openedx_tagging/import_export/test_template.py b/tests/openedx_tagging/import_export/test_template.py index 4a45f54b..5fdade21 100644 --- a/tests/openedx_tagging/import_export/test_template.py +++ b/tests/openedx_tagging/import_export/test_template.py @@ -61,9 +61,8 @@ def test_import_template(self, template_file, parser_format): ' Hi-hat (HI-HAT) (Idiophone) (children: 0)', ' Membranophone (DRUMS) (Percussion instruments) (children: 2 + 1)', ' Cajón (CAJÓN) (Membranophone) (children: 1)', - # This tag is present in the import files, but it will be omitted from get_tags() - # because it sits beyond TAXONOMY_MAX_DEPTH. - # Pyle Stringed Jam Cajón (PYLE) (Cajón) (children: 0) + # With TAXONOMY_MAX_DEPTH=3 and 0-indexed depths, this depth=3 tag is included. + ' Pyle Stringed Jam Cajón (PYLE) (Cajón) (children: 0)', ' Tabla (TABLA) (Membranophone) (children: 0)', 'String instruments (STRINGS) (None) (children: 2 + 5)', ' Bowed strings (BOW) (String instruments) (children: 2)', diff --git a/tests/openedx_tagging/test_api.py b/tests/openedx_tagging/test_api.py index aaa372b4..9b41404b 100644 --- a/tests/openedx_tagging/test_api.py +++ b/tests/openedx_tagging/test_api.py @@ -147,7 +147,8 @@ def test_get_tags(self) -> None: "Eukaryota (children: 5 + 8)", " Animalia (children: 7 + 1)", " Arthropoda (children: 0)", - " Chordata (children: 1)", # The child of this is excluded due to depth limit + " Chordata (children: 1)", + " Mammalia (children: 0)", " Cnidaria (children: 0)", " Ctenophora (children: 0)", " Gastrotrich (children: 0)", @@ -784,11 +785,12 @@ def get_object_tags(): "Bacteria (used: 0, 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: 0, children: 4 + 8)", + " Animalia (used: 1, children: 7 + 1)", " 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. + " 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)", diff --git a/tests/openedx_tagging/test_views.py b/tests/openedx_tagging/test_views.py index 67b975e0..63de0715 100644 --- a/tests/openedx_tagging/test_views.py +++ b/tests/openedx_tagging/test_views.py @@ -1459,6 +1459,7 @@ def test_small_taxonomy(self): " Animalia (Eukaryota) (children: 7 + 1)", " Arthropoda (Animalia) (children: 0)", " Chordata (Animalia) (children: 1)", + " Mammalia (Chordata) (children: 0)", " Cnidaria (Animalia) (children: 0)", " Ctenophora (Animalia) (children: 0)", " Gastrotrich (Animalia) (children: 0)", From 029a7eb854e13e5039c9c47fe949f17d08ec8ef6 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Fri, 20 Mar 2026 10:25:51 -0400 Subject: [PATCH 6/6] refactor: PR comment --- src/openedx_tagging/models/base.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/openedx_tagging/models/base.py b/src/openedx_tagging/models/base.py index 27a8592d..4daa3379 100644 --- a/src/openedx_tagging/models/base.py +++ b/src/openedx_tagging/models/base.py @@ -519,6 +519,7 @@ def _descendants_filter(self, tag_id: int) -> Q: Helper function to get all descendant tags of a given parent tag up to the supported maximum depth. """ + assert TAXONOMY_MAX_DEPTH == 3 # If we change TAXONOMY_MAX_DEPTH we need to change this query code children = Q(parent_id=tag_id) grandchildren = Q(parent__parent_id=tag_id) great_grandchildren = Q(parent__parent__parent_id=tag_id) @@ -532,6 +533,7 @@ def _max_depth_filter(self) -> Q: """ Helper function to get all tags up to the supported maximum depth. """ + assert TAXONOMY_MAX_DEPTH == 3 # If we change TAXONOMY_MAX_DEPTH we need to change this query code return Q(parent__parent__parent__parent_id=None) def _get_filtered_tags_deep( @@ -548,8 +550,6 @@ def _get_filtered_tags_deep( # All tags (possibly below a certain tag?) in the closed taxonomy, up to depth TAXONOMY_MAX_DEPTH main_parent_id = self.tag_for_value(parent_tag_value).pk if parent_tag_value else None - assert TAXONOMY_MAX_DEPTH == 3 # If we change TAXONOMY_MAX_DEPTH we need to change this query code: - qs: models.QuerySet = self.tag_set.filter(self._max_depth_filter()) # If a main parent is specified, we only want to include its descendants, otherwise, all tags.