From 0dfbe28007b7710418652955208bb2727d4b709c Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 27 Oct 2023 10:44:12 -0700 Subject: [PATCH 01/14] chore: update a test case to use the API that now exists --- tests/openedx_tagging/core/tagging/test_models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index 8c006a14c..f10a32bdb 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -750,7 +750,7 @@ def test_is_deleted(self): ] # Delete "bacteria" from the taxonomy: - self.bacteria.delete() # TODO: add an API method for this + api.delete_tags_from_taxonomy(self.taxonomy, ["Bacteria"], with_subtags=True) assert [(t.value, t.is_deleted) for t in api.get_object_tags(object_id)] == [ (self.archaea.value, False), From 879d7e18635289f37bef999b2690d8d98312b3c3 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 30 Oct 2023 14:52:37 -0700 Subject: [PATCH 02/14] feat: add validation that Tag value cannot contain \t --- .../migrations/0013_tag_parent_blank.py | 19 +++++++++++++++++ openedx_tagging/core/tagging/models/base.py | 16 ++++++++++++++ .../core/tagging/test_models.py | 21 +++++++++++++++++++ 3 files changed, 56 insertions(+) create mode 100644 openedx_tagging/core/tagging/migrations/0013_tag_parent_blank.py diff --git a/openedx_tagging/core/tagging/migrations/0013_tag_parent_blank.py b/openedx_tagging/core/tagging/migrations/0013_tag_parent_blank.py new file mode 100644 index 000000000..d947bbd31 --- /dev/null +++ b/openedx_tagging/core/tagging/migrations/0013_tag_parent_blank.py @@ -0,0 +1,19 @@ +# Generated by Django 3.2.22 on 2023-10-30 21:51 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('oel_tagging', '0012_language_taxonomy'), + ] + + operations = [ + migrations.AlterField( + model_name='tag', + name='parent', + field=models.ForeignKey(blank=True, default=None, help_text='Tag that lives one level up from the current tag, forming a hierarchy.', null=True, on_delete=django.db.models.deletion.CASCADE, related_name='children', to='oel_tagging.tag'), + ), + ] diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index 71318a13b..15090e5f3 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -51,6 +51,7 @@ class Tag(models.Model): parent = models.ForeignKey( "self", null=True, + blank=True, default=None, on_delete=models.CASCADE, related_name="children", @@ -149,6 +150,20 @@ def child_count(self) -> int: return self.taxonomy.tag_set.filter(parent=self).count() return 0 + def clean(self): + """ + Validate this tag before saving + """ + # Don't allow leading or trailing whitespace: + self.value = self.value.strip() + if self.external_id: + self.external_id = self.external_id.strip() + # Don't allow \t (tab) character at all, as we use it for lineage in database queries + if "\t" in self.value: + raise ValidationError("Tags in a taxonomy cannot contain a TAB character.") + if self.external_id and "\t" in self.external_id: + raise ValidationError("Tag external ID cannot contain a TAB character.") + class Taxonomy(models.Model): """ @@ -534,6 +549,7 @@ def add_tag( tag = Tag.objects.create( taxonomy=self, value=tag_value, parent=parent, external_id=external_id ) + tag.full_clean() return tag diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index f10a32bdb..34bd5c20e 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -210,6 +210,27 @@ def test_unique_tags(self): def test_get_lineage(self, tag_attr, lineage): assert getattr(self, tag_attr).get_lineage() == lineage + def test_trailing_whitespace(self): + """ + Test that tags automatically strip out trailing/leading whitespace + """ + t = self.taxonomy.add_tag(" white space ") + assert t.value == "white space" + # And via the API: + t2 = api.add_tag_to_taxonomy(self.taxonomy, "\t value\n") + assert t2.value == "value" + + def test_no_tab(self): + """ + Test that tags cannot contain a TAB character, which we use as a field + separator in the database when computing lineage. + """ + with pytest.raises(ValidationError): + self.taxonomy.add_tag("has\ttab") + # And via the API: + with pytest.raises(ValidationError): + api.add_tag_to_taxonomy(self.taxonomy, "first\tsecond") + @ddt.ddt class TestFilteredTagsClosedTaxonomy(TestTagTaxonomyMixin, TestCase): From bfa6a0a66228827ebfea4e88991158aaa19d3e80 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 1 Nov 2023 11:26:00 -0700 Subject: [PATCH 03/14] feat: add validation that Object ID cannot contain "," or "*" --- openedx_tagging/core/tagging/models/base.py | 3 +++ tests/openedx_tagging/core/tagging/test_models.py | 12 ++++++++++++ 2 files changed, 15 insertions(+) diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index 15090e5f3..9644c7976 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -818,6 +818,9 @@ def clean(self): raise ValidationError("Invalid _value - empty string") if self.taxonomy and self.taxonomy.name != self._name: raise ValidationError("ObjectTag's _name is out of sync with Taxonomy.name") + if "," in self.object_id or "*" in self.object_id: + # Some APIs may use these characters to allow wildcard matches or multiple matches in the future. + raise ValidationError("Object ID contains invalid characters") def get_lineage(self) -> Lineage: """ diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index 34bd5c20e..c1ea62557 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -751,6 +751,18 @@ def test_tag_case(self) -> None: tag=self.chordata, ).save() + def test_invalid_id(self): + """ + Test attempting to create object tags with invalid characters in the object ID + """ + open_taxonomy = Taxonomy.objects.create(name="Freetext", allow_free_text=True, allow_multiple=True) + args = {"tags": ["test"], "taxonomy": open_taxonomy} + with pytest.raises(ValidationError): + api.tag_object(object_id="wildcard*", **args) + with pytest.raises(ValidationError): + api.tag_object(object_id="one,two,three", **args) + api.tag_object(object_id="valid", **args) + def test_is_deleted(self): self.taxonomy.allow_multiple = True self.taxonomy.save() From 6264adf9a8b04d5d110aca610102b15f2d925371 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 31 Oct 2023 13:35:57 -0700 Subject: [PATCH 04/14] chore: clean up test_views --- .../core/tagging/test_views.py | 126 +++++++----------- 1 file changed, 51 insertions(+), 75 deletions(-) diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 6a3125464..efbd17019 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -12,10 +12,11 @@ from rest_framework import status from rest_framework.test import APITestCase +from openedx_tagging.core.tagging import api from openedx_tagging.core.tagging.import_export import api as import_export_api from openedx_tagging.core.tagging.import_export.parsers import ParserFormat from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy -from openedx_tagging.core.tagging.models.system_defined import SystemDefinedTaxonomy +from openedx_tagging.core.tagging.models.system_defined import SystemDefinedTaxonomy, UserSystemDefinedTaxonomy from openedx_tagging.core.tagging.rest_api.paginators import TagsPagination from openedx_tagging.core.tagging.rules import can_change_object_tag_objectid, can_view_object_tag_objectid @@ -136,11 +137,11 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int): def test_list_taxonomy_pagination(self) -> None: url = TAXONOMY_LIST_URL - Taxonomy.objects.create(name="T1", enabled=True).save() - Taxonomy.objects.create(name="T2", enabled=True).save() - Taxonomy.objects.create(name="T3", enabled=False).save() - Taxonomy.objects.create(name="T4", enabled=False).save() - Taxonomy.objects.create(name="T5", enabled=False).save() + api.create_taxonomy(name="T1", enabled=True).save() + api.create_taxonomy(name="T2", enabled=True).save() + api.create_taxonomy(name="T3", enabled=False).save() + api.create_taxonomy(name="T4", enabled=False).save() + api.create_taxonomy(name="T5", enabled=False).save() self.client.force_authenticate(user=self.staff) @@ -177,7 +178,7 @@ def test_list_invalid_page(self) -> None: @ddt.unpack def test_detail_taxonomy(self, user_attr: str | None, taxonomy_data: dict[str, bool], expected_status: int): create_data = {"name": "taxonomy detail test", **taxonomy_data} - taxonomy = Taxonomy.objects.create(**create_data) + taxonomy = api.create_taxonomy(**create_data) url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) if user_attr: @@ -265,12 +266,11 @@ def test_create_taxonomy_system_defined(self, create_data): ) @ddt.unpack def test_update_taxonomy(self, user_attr, expected_status): - taxonomy = Taxonomy.objects.create( + taxonomy = api.create_taxonomy( name="test update taxonomy", description="taxonomy description", enabled=True, ) - taxonomy.save() url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) @@ -303,10 +303,10 @@ def test_update_taxonomy_system_defined(self, system_defined, expected_status): """ Test that we can't update system_defined field """ - taxonomy = Taxonomy.objects.create(name="test system taxonomy") - if system_defined: - taxonomy.taxonomy_class = SystemDefinedTaxonomy - taxonomy.save() + taxonomy = api.create_taxonomy( + name="test system taxonomy", + taxonomy_class=SystemDefinedTaxonomy if system_defined else None, + ) url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) self.client.force_authenticate(user=self.staff) @@ -327,8 +327,7 @@ def test_update_taxonomy_404(self): ) @ddt.unpack def test_patch_taxonomy(self, user_attr, expected_status): - taxonomy = Taxonomy.objects.create(name="test patch taxonomy", enabled=False) - taxonomy.save() + taxonomy = api.create_taxonomy(name="test patch taxonomy", enabled=False) url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) @@ -360,10 +359,10 @@ def test_patch_taxonomy_system_defined(self, system_defined, expected_status): """ Test that we can't patch system_defined field """ - taxonomy = Taxonomy.objects.create(name="test system taxonomy") - if system_defined: - taxonomy.taxonomy_class = SystemDefinedTaxonomy - taxonomy.save() + taxonomy = api.create_taxonomy( + name="test system taxonomy", + taxonomy_class=SystemDefinedTaxonomy if system_defined else None, + ) url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) self.client.force_authenticate(user=self.staff) @@ -384,8 +383,7 @@ def test_patch_taxonomy_404(self): ) @ddt.unpack def test_delete_taxonomy(self, user_attr, expected_status): - taxonomy = Taxonomy.objects.create(name="test delete taxonomy") - taxonomy.save() + taxonomy = api.create_taxonomy(name="test delete taxonomy") url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) @@ -417,8 +415,7 @@ def test_export_taxonomy(self, output_format, content_type): """ Tests if a user can export a taxonomy """ - taxonomy = Taxonomy.objects.create(name="T1", enabled=True) - taxonomy.save() + taxonomy = api.create_taxonomy(name="T1") for i in range(20): # Valid ObjectTags Tag.objects.create(taxonomy=taxonomy, value=f"Tag {i}").save() @@ -445,11 +442,9 @@ def test_export_taxonomy_download(self, output_format, content_type): """ Tests if a user can export a taxonomy with download option """ - taxonomy = Taxonomy.objects.create(name="T1", enabled=True) - taxonomy.save() + taxonomy = api.create_taxonomy(name="T1") for i in range(20): - # Valid ObjectTags - Tag.objects.create(taxonomy=taxonomy, value=f"Tag {i}").save() + api.add_tag_to_taxonomy(taxonomy=taxonomy, tag=f"Tag {i}") url = TAXONOMY_EXPORT_URL.format(pk=taxonomy.pk) @@ -469,8 +464,7 @@ def test_export_taxonomy_invalid_param_output_format(self): """ Tests if a user can export a taxonomy using an invalid output_format param """ - taxonomy = Taxonomy.objects.create(name="T1", enabled=True) - taxonomy.save() + taxonomy = api.create_taxonomy(name="T1") url = TAXONOMY_EXPORT_URL.format(pk=taxonomy.pk) @@ -482,8 +476,7 @@ def test_export_taxonomy_invalid_param_download(self): """ Tests if a user can export a taxonomy using an invalid output_format param """ - taxonomy = Taxonomy.objects.create(name="T1", enabled=True) - taxonomy.save() + taxonomy = api.create_taxonomy(name="T1") url = TAXONOMY_EXPORT_URL.format(pk=taxonomy.pk) @@ -496,8 +489,7 @@ def test_export_taxonomy_unauthorized(self): Tests if a user can export a taxonomy that he doesn't have authorization """ # Only staff can view a disabled taxonomy - taxonomy = Taxonomy.objects.create(name="T1", enabled=False) - taxonomy.save() + taxonomy = api.create_taxonomy(name="T1", enabled=False) url = TAXONOMY_EXPORT_URL.format(pk=taxonomy.pk) @@ -548,67 +540,45 @@ def _view_object_permission(user, object_id: str) -> bool: ) # System-defined language taxonomy with valid ObjectTag - self.system_taxonomy = SystemDefinedTaxonomy.objects.create(name="System Taxonomy") - self.tag1 = Tag.objects.create(taxonomy=self.system_taxonomy, value="Tag 1") - ObjectTag.objects.create(object_id="abc", taxonomy=self.system_taxonomy, tag=self.tag1) + self.author_sys_taxonomy = api.create_taxonomy(taxonomy_class=UserSystemDefinedTaxonomy, name="Author") + # Tag the object "abc" with the tag for self.staff + api.tag_object(object_id="abc", taxonomy=self.author_sys_taxonomy, tags=[self.staff.username]) # Language system-defined language taxonomy self.language_taxonomy = Taxonomy.objects.get(pk=LANGUAGE_TAXONOMY_ID) # Closed Taxonomies created by taxonomy admins, each with 20 ObjectTags - self.enabled_taxonomy = Taxonomy.objects.create(name="Enabled Taxonomy", allow_multiple=False) - self.disabled_taxonomy = Taxonomy.objects.create(name="Disabled Taxonomy", enabled=False, allow_multiple=False) - self.multiple_taxonomy = Taxonomy.objects.create(name="Multiple Taxonomy", allow_multiple=True) + self.enabled_taxonomy = api.create_taxonomy(name="Enabled Taxonomy", allow_multiple=False) + self.disabled_taxonomy = api.create_taxonomy(name="Disabled Taxonomy", enabled=False, allow_multiple=False) + self.multiple_taxonomy = api.create_taxonomy(name="Multiple Taxonomy", allow_multiple=True) for i in range(20): - # Valid ObjectTags - tag_enabled = Tag.objects.create(taxonomy=self.enabled_taxonomy, value=f"Tag {i}") - tag_disabled = Tag.objects.create(taxonomy=self.disabled_taxonomy, value=f"Tag {i}") - tag_multiple = Tag.objects.create(taxonomy=self.multiple_taxonomy, value=f"Tag {i}") - ObjectTag.objects.create( - object_id="abc", taxonomy=self.enabled_taxonomy, tag=tag_enabled, _value=tag_enabled.value - ) - ObjectTag.objects.create( - object_id="abc", taxonomy=self.disabled_taxonomy, tag=tag_disabled, _value=tag_disabled.value - ) - ObjectTag.objects.create( - object_id="abc", taxonomy=self.multiple_taxonomy, tag=tag_multiple, _value=tag_multiple.value - ) + api.add_tag_to_taxonomy(taxonomy=self.enabled_taxonomy, tag=f"Tag {i}") + api.add_tag_to_taxonomy(taxonomy=self.disabled_taxonomy, tag=f"Tag {i}") + api.add_tag_to_taxonomy(taxonomy=self.multiple_taxonomy, tag=f"Tag {i}") + api.tag_object(object_id="abc", taxonomy=self.enabled_taxonomy, tags=["Tag 15"]) + api.tag_object(object_id="abc", taxonomy=self.disabled_taxonomy, tags=["Tag 3"]) + api.tag_object(object_id="abc", taxonomy=self.multiple_taxonomy, tags=["Tag 4", "Tag 7", "Tag 12"]) # Free-Text Taxonomies created by taxonomy admins, each linked # to 10 ObjectTags - self.open_taxonomy_enabled = Taxonomy.objects.create( + self.open_taxonomy_enabled = api.create_taxonomy( name="Enabled Free-Text Taxonomy", allow_free_text=True, allow_multiple=False, ) - self.open_taxonomy_disabled = Taxonomy.objects.create( + self.open_taxonomy_disabled = api.create_taxonomy( name="Disabled Free-Text Taxonomy", allow_free_text=True, enabled=False, allow_multiple=False, ) for i in range(10): ObjectTag.objects.create(object_id="abc", taxonomy=self.open_taxonomy_enabled, _value=f"Free Text {i}") ObjectTag.objects.create(object_id="abc", taxonomy=self.open_taxonomy_disabled, _value=f"Free Text {i}") - self.dummy_taxonomies = [] - for i in range(100): - taxonomy = Taxonomy.objects.create( - name=f"Dummy Taxonomy {i}", - allow_free_text=True, - allow_multiple=True - ) - ObjectTag.objects.create( - object_id="limit_tag_count", - taxonomy=taxonomy, - _name=taxonomy.name, - _value="Dummy Tag" - ) - self.dummy_taxonomies.append(taxonomy) - # Override the object permission for the test rules.set_perm("oel_tagging.change_objecttag_objectid", _change_object_permission) rules.set_perm("oel_tagging.view_objecttag_objectid", _view_object_permission) @ddt.data( (None, status.HTTP_401_UNAUTHORIZED, None), - ("user", status.HTTP_200_OK, 81), - ("staff", status.HTTP_200_OK, 81), + ("user", status.HTTP_200_OK, 26), + ("staff", status.HTTP_200_OK, 26), ) @ddt.unpack def test_retrieve_object_tags(self, user_attr, expected_status, expected_count): @@ -638,8 +608,8 @@ def test_retrieve_object_tags_unauthorized(self): @ddt.data( (None, "abc", status.HTTP_401_UNAUTHORIZED, None), - ("user", "abc", status.HTTP_200_OK, 20), - ("staff", "abc", status.HTTP_200_OK, 20), + ("user", "abc", status.HTTP_200_OK, 1), + ("staff", "abc", status.HTTP_200_OK, 1), ) @ddt.unpack def test_retrieve_object_tags_taxonomy_queryparam( @@ -885,6 +855,12 @@ def test_tag_object_count_limit(self): Checks if the limit of 100 tags per object is enforced """ object_id = "limit_tag_count" + dummy_taxonomies = [] + for i in range(100): + taxonomy = api.create_taxonomy(name=f"Dummy Taxonomy {i}", allow_free_text=True, allow_multiple=True) + api.tag_object(object_id=object_id, taxonomy=taxonomy, tags=["Dummy Tag"]) + dummy_taxonomies.append(taxonomy) + url = OBJECT_TAGS_UPDATE_URL.format(object_id=object_id, taxonomy_id=self.enabled_taxonomy.pk) self.client.force_authenticate(user=self.staff) response = self.client.put(url, {"tags": ["Tag 1"]}, format="json") @@ -892,13 +868,13 @@ def test_tag_object_count_limit(self): assert response.status_code == status.HTTP_400_BAD_REQUEST # The user can edit the tags that are already on the object - for taxonomy in self.dummy_taxonomies: + for taxonomy in dummy_taxonomies: url = OBJECT_TAGS_UPDATE_URL.format(object_id=object_id, taxonomy_id=taxonomy.pk) response = self.client.put(url, {"tags": ["New Tag"]}, format="json") assert response.status_code == status.HTTP_200_OK # Editing tags adding another one will fail - for taxonomy in self.dummy_taxonomies: + for taxonomy in dummy_taxonomies: url = OBJECT_TAGS_UPDATE_URL.format(object_id=object_id, taxonomy_id=taxonomy.pk) response = self.client.put(url, {"tags": ["New Tag 1", "New Tag 2"]}, format="json") assert response.status_code == status.HTTP_400_BAD_REQUEST From a1a9565766cadfa9394ca6a5b02e2274f9905d1e Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 31 Oct 2023 14:17:02 -0700 Subject: [PATCH 05/14] feat: improve get_lineage() efficiency and remove depth limit --- openedx_tagging/core/tagging/models/base.py | 16 +++++++--------- .../core/tagging/rest_api/v1/serializers.py | 3 +++ .../openedx_tagging/core/tagging/test_models.py | 4 ++-- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index 9644c7976..446d5cb88 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -101,16 +101,14 @@ def get_lineage(self) -> Lineage: Queries and returns the lineage of the current tag as a list of Tag.value strings. The root Tag.value is first, followed by its child.value, and on down to self.value. - - Performance note: may perform as many as TAXONOMY_MAX_DEPTH select queries. """ - lineage: Lineage = [] - tag: Tag | None = self - depth = TAXONOMY_MAX_DEPTH - while tag and depth > 0: - lineage.insert(0, tag.value) - tag = tag.parent - depth -= 1 + lineage: Lineage = [self.value] + if self.parent_id: + # Get parent, grandparent, and great-grandparent in one query: + next_ancestor = Tag.objects.select_related("parent", "parent__parent").get(pk=self.parent_id) + while next_ancestor: + lineage.insert(0, next_ancestor.value) + next_ancestor = next_ancestor.parent return lineage @cached_property diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index 4fe62c72a..aba56a310 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -69,13 +69,16 @@ class ObjectTagSerializer(serializers.ModelSerializer): class Meta: model = ObjectTag fields = [ + # The taxonomy name "name", "value", + "lineage", "taxonomy_id", # If the Tag or Taxonomy has been deleted, this ObjectTag shouldn't be shown to users. "is_deleted", ] + lineage = serializers.ListField(child=serializers.CharField(), source="get_lineage") class ObjectTagUpdateBodySerializer(serializers.Serializer): # pylint: disable=abstract-method """ diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index c1ea62557..f0fa1639d 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -203,8 +203,8 @@ def test_unique_tags(self): ("eubacteria", ["Bacteria", "Eubacteria"]), # Third level tags return three levels ("chordata", ["Eukaryota", "Animalia", "Chordata"]), - # Lineage beyond TAXONOMY_MAX_DEPTH won't trace back to the root - ("mammalia", ["Animalia", "Chordata", "Mammalia"]), + # Even fourth level tags work + ("mammalia", ["Eukaryota", "Animalia", "Chordata", "Mammalia"]), ) @ddt.unpack def test_get_lineage(self, tag_attr, lineage): From f37e9f260a30ea73fe5a4c3b03355e5302412cf5 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 31 Oct 2023 14:40:56 -0700 Subject: [PATCH 06/14] feat: improve get_lineage() further --- openedx_tagging/core/tagging/models/base.py | 26 ++++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index 446d5cb88..68d1431ff 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -103,13 +103,27 @@ def get_lineage(self) -> Lineage: The root Tag.value is first, followed by its child.value, and on down to self.value. """ lineage: Lineage = [self.value] - if self.parent_id: - # Get parent, grandparent, and great-grandparent in one query: - next_ancestor = Tag.objects.select_related("parent", "parent__parent").get(pk=self.parent_id) - while next_ancestor: - lineage.insert(0, next_ancestor.value) - next_ancestor = next_ancestor.parent + next_ancestor = self.get_next_ancestor() + while next_ancestor: + lineage.insert(0, next_ancestor.value) + next_ancestor = next_ancestor.get_next_ancestor() return lineage + + def get_next_ancestor(self) -> Tag | None: + """ + Fetch the parent of this Tag. + + While doing so, preload several ancestors at the same time, so we can + use fewer database queries than the basic approach of iterating through + parent.parent.parent... + """ + if self.parent_id is None: + return None + if not Tag.parent.is_cached(self): + # Parent is not yet loaded. Retrieve our parent, grandparent, and great-grandparent in one query. + # This is not actually changing the parent, just loading it and caching it. + self.parent = Tag.objects.select_related("parent", "parent__parent").get(pk=self.parent_id) + return self.parent @cached_property def depth(self) -> int: From 9966402d5e18549d98eb6aac1de347da06a642ef Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 31 Oct 2023 14:17:13 -0700 Subject: [PATCH 07/14] feat: retrieve object tags along with their lineage, grouped by taxonomy --- openedx_tagging/core/tagging/api.py | 18 +- openedx_tagging/core/tagging/models/base.py | 6 +- .../core/tagging/rest_api/v1/serializers.py | 49 ++- .../core/tagging/rest_api/v1/views.py | 40 +- .../openedx_tagging/core/tagging/test_api.py | 39 +- .../core/tagging/test_models.py | 118 +++-- .../core/tagging/test_views.py | 402 ++++++++++-------- 7 files changed, 379 insertions(+), 293 deletions(-) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index a655cd783..f0f23eb54 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -12,12 +12,14 @@ """ from __future__ import annotations -from django.db import transaction -from django.db.models import QuerySet +from django.db import models, transaction +from django.db.models import F, QuerySet, Value +from django.db.models.functions import Coalesce, Concat, Lower from django.utils.translation import gettext as _ from .data import TagDataQuerySet from .models import ObjectTag, Tag, Taxonomy +from .models.utils import ConcatNull # Export this as part of the API TagDoesNotExist = Tag.DoesNotExist @@ -165,8 +167,16 @@ def get_object_tags( filters = {"taxonomy_id": taxonomy_id} if taxonomy_id else {} tags = ( object_tag_class.objects.filter(object_id=object_id, **filters) - .select_related("tag", "taxonomy") - .order_by("id") + .select_related("taxonomy", "tag", "tag__parent", "tag__parent__parent") + .annotate(sort_key=Lower(Concat( + ConcatNull(F("tag__parent__parent__parent__value"), Value("\t")), + ConcatNull(F("tag__parent__parent__value"), Value("\t")), + ConcatNull(F("tag__parent__value"), Value("\t")), + Coalesce(F("tag__value"), F("_value")), + Value("\t"), + output_field=models.CharField(), + ))) + .order_by("sort_key") ) return tags diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index 68d1431ff..86f948770 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -108,18 +108,18 @@ def get_lineage(self) -> Lineage: lineage.insert(0, next_ancestor.value) next_ancestor = next_ancestor.get_next_ancestor() return lineage - + def get_next_ancestor(self) -> Tag | None: """ Fetch the parent of this Tag. - + While doing so, preload several ancestors at the same time, so we can use fewer database queries than the basic approach of iterating through parent.parent.parent... """ if self.parent_id is None: return None - if not Tag.parent.is_cached(self): + if not Tag.parent.is_cached(self): # pylint: disable=no-member # Parent is not yet loaded. Retrieve our parent, grandparent, and great-grandparent in one query. # This is not actually changing the parent, just loading it and caching it. self.parent = Tag.objects.select_related("parent", "parent__parent").get(pk=self.parent_id) diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index aba56a310..a8fb7a94e 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -3,6 +3,8 @@ """ from __future__ import annotations +from typing import Any + from rest_framework import serializers from rest_framework.reverse import reverse @@ -61,24 +63,59 @@ class ObjectTagListQueryParamsSerializer(serializers.Serializer): # pylint: dis ) -class ObjectTagSerializer(serializers.ModelSerializer): +class ObjectTagMinimalSerializer(serializers.ModelSerializer): """ - Serializer for the ObjectTag model. + Minimal serializer for the ObjectTag model. """ class Meta: model = ObjectTag - fields = [ + fields = ["value", "lineage"] + + lineage = serializers.ListField(child=serializers.CharField(), source="get_lineage") + + +class ObjectTagSerializer(ObjectTagMinimalSerializer): + """ + Serializer for the ObjectTag model. + """ + class Meta: + fields = ObjectTagMinimalSerializer.Meta.fields + [ # The taxonomy name "name", - "value", - "lineage", "taxonomy_id", # If the Tag or Taxonomy has been deleted, this ObjectTag shouldn't be shown to users. "is_deleted", ] - lineage = serializers.ListField(child=serializers.CharField(), source="get_lineage") + +class ObjectTagsByTaxonomySerializer(serializers.ModelSerializer): + """ + Serialize a group of ObjectTags, grouped by taxonomy + """ + def to_representation(self, instance: list[ObjectTag]) -> dict: + """ + Convert this list of ObjectTags to the serialized dictionary, grouped by Taxonomy + """ + by_object: dict[str, dict[str, Any]] = {} + for obj_tag in instance: + if obj_tag.object_id not in by_object: + by_object[obj_tag.object_id] = { + "taxonomies": [] + } + taxonomies = by_object[obj_tag.object_id]["taxonomies"] + tax_entry = next((t for t in taxonomies if t["taxonomy_id"] == obj_tag.taxonomy_id), None) + if tax_entry is None: + tax_entry = { + "name": obj_tag.name, + "taxonomy_id": obj_tag.taxonomy_id, + "editable": (not obj_tag.taxonomy.cast().system_defined) if obj_tag.taxonomy else False, + "tags": [] + } + taxonomies.append(tax_entry) + tax_entry["tags"].append(ObjectTagMinimalSerializer(obj_tag).data) + return by_object + class ObjectTagUpdateBodySerializer(serializers.Serializer): # pylint: disable=abstract-method """ diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index 951c8e6f2..7b6200f8c 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -32,6 +32,7 @@ from .permissions import ObjectTagObjectPermissions, TagObjectPermissions, TaxonomyObjectPermissions from .serializers import ( ObjectTagListQueryParamsSerializer, + ObjectTagsByTaxonomySerializer, ObjectTagSerializer, ObjectTagUpdateBodySerializer, ObjectTagUpdateQueryParamsSerializer, @@ -306,20 +307,20 @@ def get_queryset(self) -> models.QuerySet: taxonomy = taxonomy.cast() taxonomy_id = taxonomy.id - perm = "oel_tagging.view_objecttag" - perm_obj = ObjectTagPermissionItem( - taxonomy=taxonomy, - object_id=object_id, - ) - - if not self.request.user.has_perm( - perm, - # The obj arg expects a model, but we are passing an object - perm_obj, # type: ignore[arg-type] - ): - raise PermissionDenied( - "You do not have permission to view object tags for this taxonomy or object_id." - ) + if object_id.endswith("*") or "," in object_id: # pylint: disable=no-else-raise + raise ValidationError("Retrieving tags from multiple objects is not yet supported.") + # Note: This API is actually designed so that in the future it can be extended to return tags for multiple + # objects, e.g. if object_id.endswith("*") then it results in a object_id__startswith query. However, for + # now we have no use case for that so we retrieve tags for one object at a time. + else: + if not self.request.user.has_perm( + "oel_tagging.view_objecttag", + # The obj arg expects a model, but we are passing an object + ObjectTagPermissionItem(taxonomy=taxonomy, object_id=object_id), # type: ignore[arg-type] + ): + raise PermissionDenied( + "You do not have permission to view object tags for this taxonomy or object_id." + ) return get_object_tags(object_id, taxonomy_id) @@ -334,9 +335,14 @@ def retrieve(self, request, *args, **kwargs) -> Response: path and returns a it as a single result however that is not behavior we want. """ - object_tags = self.filter_queryset(self.get_queryset()) - serializer = ObjectTagSerializer(object_tags, many=True) - return Response(serializer.data) + object_tags = self.filter_queryset(self.get_queryset()).order_by("sort_key") + serializer = ObjectTagsByTaxonomySerializer(list(object_tags)) + response_data = serializer.data + if self.kwargs["object_id"] not in response_data: + # For consistency, the key with the object_id should always be present in the response, even if there + # are no tags at all applied to this object. + response_data[self.kwargs["object_id"]] = {"taxonomies": []} + return Response(response_data) def update(self, request, *args, **kwargs) -> Response: """ diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 909495a4e..a488cc824 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -85,17 +85,19 @@ def test_get_taxonomies(self) -> None: enabled = list(tagging_api.get_taxonomies()) assert enabled == [ tax1, + self.free_text_taxonomy, tax3, self.language_taxonomy, self.taxonomy, self.system_taxonomy, self.user_taxonomy, - ] + self.dummy_taxonomies + ] assert str(enabled[0]) == f" ({tax1.id}) Enabled" - assert str(enabled[1]) == " (5) Import Taxonomy Test" - assert str(enabled[2]) == " (-1) Languages" - assert str(enabled[3]) == " (1) Life on Earth" - assert str(enabled[4]) == " (4) System defined taxonomy" + assert str(enabled[1]) == " (6) Free Text" + assert str(enabled[2]) == " (5) Import Taxonomy Test" + assert str(enabled[3]) == " (-1) Languages" + assert str(enabled[4]) == " (1) Life on Earth" + assert str(enabled[5]) == " (4) System defined taxonomy" with self.assertNumQueries(1): disabled = list(tagging_api.get_taxonomies(enabled=False)) @@ -107,12 +109,13 @@ def test_get_taxonomies(self) -> None: assert both == [ tax2, tax1, + self.free_text_taxonomy, tax3, self.language_taxonomy, self.taxonomy, self.system_taxonomy, self.user_taxonomy, - ] + self.dummy_taxonomies + ] def test_get_tags(self) -> None: assert pretty_format_tags(tagging_api.get_tags(self.taxonomy), parent=False) == [ @@ -265,18 +268,18 @@ def test_resync_object_tags(self) -> None: assert [(t.value, t.is_deleted) for t in tagging_api.get_object_tags(object_id)] == [ (self.archaea.value, False), (self.bacteria.value, False), - ("foo", False), ("bar", False), + ("foo", False), ] # Delete "bacteria" from the taxonomy: - self.bacteria.delete() # TODO: add an API method for this + tagging_api.delete_tags_from_taxonomy(self.taxonomy, [self.bacteria.value], with_subtags=True) assert [(t.value, t.is_deleted) for t in tagging_api.get_object_tags(object_id)] == [ (self.archaea.value, False), (self.bacteria.value, True), # <--- deleted! But the value is preserved. - ("foo", False), ("bar", False), + ("foo", False), ] # Re-syncing the tags at this point does nothing: @@ -293,8 +296,8 @@ def test_resync_object_tags(self) -> None: assert [(t.value, t.is_deleted) for t in tagging_api.get_object_tags(object_id)] == [ (self.archaea.value, False), (self.bacteria.value, False), # <--- not deleted - ("foo", False), ("bar", False), + ("foo", False), ] # Re-syncing the tags now does nothing: @@ -311,12 +314,12 @@ def test_tag_object(self): self.chordata, ], [ - self.chordata, self.archaebacteria, + self.chordata, ], [ - self.archaebacteria, self.archaea, + self.archaebacteria, ], ] @@ -520,8 +523,9 @@ def test_tag_object_limit(self) -> None: """ Test that the tagging limit is enforced. """ + dummy_taxonomies = self.create_100_taxonomies() # The user can add up to 100 tags to a object - for taxonomy in self.dummy_taxonomies: + for taxonomy in dummy_taxonomies: tagging_api.tag_object( taxonomy, ["Dummy Tag"], @@ -539,7 +543,7 @@ def test_tag_object_limit(self) -> None: assert "Cannot add more than 100 tags to" in str(exc.exception) # Updating existing tags should work - for taxonomy in self.dummy_taxonomies: + for taxonomy in dummy_taxonomies: tagging_api.tag_object( taxonomy, ["New Dummy Tag"], @@ -547,7 +551,7 @@ def test_tag_object_limit(self) -> None: ) # Updating existing tags adding a new one should fail - for taxonomy in self.dummy_taxonomies: + for taxonomy in dummy_taxonomies: with self.assertRaises(ValueError) as exc: tagging_api.tag_object( taxonomy, @@ -558,15 +562,16 @@ def test_tag_object_limit(self) -> None: assert "Cannot add more than 100 tags to" in str(exc.exception) def test_get_object_tags(self) -> None: - # Alpha tag has no taxonomy + # Alpha tag has no taxonomy (as if the taxonomy had been deleted) alpha = ObjectTag(object_id="abc") alpha.name = self.taxonomy.name - alpha.value = self.mammalia.value + alpha.value = "alpha" alpha.save() # Beta tag has a closed taxonomy beta = ObjectTag.objects.create( object_id="abc", taxonomy=self.taxonomy, + tag=self.taxonomy.tag_set.get(value="Protista"), ) # Fetch all the tags for a given object ID diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index f0fa1639d..7d4698519 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -33,12 +33,14 @@ class TestTagTaxonomyMixin: def setUp(self): super().setUp() + # Core pre-defined taxonomies for testing: self.taxonomy = Taxonomy.objects.get(name="Life on Earth") - self.system_taxonomy = Taxonomy.objects.get( - name="System defined taxonomy" - ) + self.system_taxonomy = Taxonomy.objects.get(name="System defined taxonomy") self.language_taxonomy = LanguageTaxonomy.objects.get(name="Languages") self.user_taxonomy = Taxonomy.objects.get(name="User Authors").cast() + self.free_text_taxonomy = Taxonomy.objects.create(name="Free Text", allow_free_text=True) + + # References to some tags: self.archaea = get_tag("Archaea") self.archaebacteria = get_tag("Archaebacteria") self.bacteria = get_tag("Bacteria") @@ -73,7 +75,41 @@ def setUp(self): get_tag("System Tag 4"), ] - self.dummy_taxonomies = [] + def create_sort_test_taxonomy(self) -> Taxonomy: + """ + Helper method to create a taxonomy that's difficult to sort correctly in tree order. + """ + # pylint: disable=unused-variable + taxonomy = api.create_taxonomy("Sort Test") + + root1 = Tag.objects.create(taxonomy=taxonomy, value="1") + child1_1 = Tag.objects.create(taxonomy=taxonomy, value="11", parent=root1) + child1_2 = Tag.objects.create(taxonomy=taxonomy, value="2", parent=root1) + child1_3 = Tag.objects.create(taxonomy=taxonomy, value="1 A", parent=root1) + child1_4 = Tag.objects.create(taxonomy=taxonomy, value="11111", parent=root1) + grandchild1_4_1 = Tag.objects.create(taxonomy=taxonomy, value="1111-grandchild", parent=child1_4) + + root2 = Tag.objects.create(taxonomy=taxonomy, value="111") + child2_1 = Tag.objects.create(taxonomy=taxonomy, value="11111111", parent=root2) + child2_2 = Tag.objects.create(taxonomy=taxonomy, value="123", parent=root2) + + root1 = Tag.objects.create(taxonomy=taxonomy, value="ALPHABET") + child1_1 = Tag.objects.create(taxonomy=taxonomy, value="Android", parent=root1) + child1_2 = Tag.objects.create(taxonomy=taxonomy, value="abacus", parent=root1) + child1_2 = Tag.objects.create(taxonomy=taxonomy, value="azure", parent=root1) + child1_3 = Tag.objects.create(taxonomy=taxonomy, value="aardvark", parent=root1) + child1_4 = Tag.objects.create(taxonomy=taxonomy, value="ANVIL", parent=root1) + + root2 = Tag.objects.create(taxonomy=taxonomy, value="abstract") + child2_1 = Tag.objects.create(taxonomy=taxonomy, value="Andes", parent=root2) + child2_2 = Tag.objects.create(taxonomy=taxonomy, value="azores islands", parent=root2) + return taxonomy + + def create_100_taxonomies(self): + """ + Helper method to create 100 taxonomies and to apply a tag from each to an object + """ + dummy_taxonomies = [] for i in range(100): taxonomy = Taxonomy.objects.create( name=f"ZZ Dummy Taxonomy {i:03}", @@ -86,7 +122,8 @@ def setUp(self): _name=taxonomy.name, _value="Dummy Tag", ) - self.dummy_taxonomies.append(taxonomy) + dummy_taxonomies.append(taxonomy) + return dummy_taxonomies class TaxonomyTestSubclassA(Taxonomy): @@ -465,22 +502,13 @@ def test_usage_count(self) -> None: "Bacteria (None) (used: 3, children: 2)", ] - def test_pathological_tree_sort(self) -> None: + def test_tree_sort(self) -> None: """ - Check for bugs in how tree sorting happens, if the tag names are very - similar. + Verify that taxonomies can be sorted correctly in tree orer (case insensitive). + + The taxonomy used contains values that are tricky to sort correctly unless the tree sort algorithm is correct. """ - # pylint: disable=unused-variable - taxonomy = api.create_taxonomy("Sort Test") - root1 = Tag.objects.create(taxonomy=taxonomy, value="1") - child1_1 = Tag.objects.create(taxonomy=taxonomy, value="11", parent=root1) - child1_2 = Tag.objects.create(taxonomy=taxonomy, value="2", parent=root1) - child1_3 = Tag.objects.create(taxonomy=taxonomy, value="1 A", parent=root1) - child1_4 = Tag.objects.create(taxonomy=taxonomy, value="11111", parent=root1) - grandchild1_4_1 = Tag.objects.create(taxonomy=taxonomy, value="1111-grandchild", parent=child1_4) - root2 = Tag.objects.create(taxonomy=taxonomy, value="111") - child2_1 = Tag.objects.create(taxonomy=taxonomy, value="11111111", parent=root2) - child2_2 = Tag.objects.create(taxonomy=taxonomy, value="123", parent=root2) + taxonomy = self.create_sort_test_taxonomy() result = pretty_format_tags(taxonomy.get_filtered_tags()) assert result == [ "1 (None) (children: 4)", @@ -492,27 +520,6 @@ def test_pathological_tree_sort(self) -> None: "111 (None) (children: 2)", " 11111111 (111) (children: 0)", " 123 (111) (children: 0)", - ] - - def test_case_insensitive_sort(self) -> None: - """ - Make sure the sorting is case-insensitive - """ - # pylint: disable=unused-variable - taxonomy = api.create_taxonomy("Sort Test") - root1 = Tag.objects.create(taxonomy=taxonomy, value="ALPHABET") - child1_1 = Tag.objects.create(taxonomy=taxonomy, value="Android", parent=root1) - child1_2 = Tag.objects.create(taxonomy=taxonomy, value="abacus", parent=root1) - child1_2 = Tag.objects.create(taxonomy=taxonomy, value="azure", parent=root1) - child1_3 = Tag.objects.create(taxonomy=taxonomy, value="aardvark", parent=root1) - child1_4 = Tag.objects.create(taxonomy=taxonomy, value="ANVIL", parent=root1) - - root2 = Tag.objects.create(taxonomy=taxonomy, value="abstract") - child2_1 = Tag.objects.create(taxonomy=taxonomy, value="Andes", parent=root2) - child2_2 = Tag.objects.create(taxonomy=taxonomy, value="azores islands", parent=root2) - - result = pretty_format_tags(taxonomy.get_filtered_tags()) - assert result == [ "abstract (None) (children: 2)", " Andes (abstract) (children: 0)", " azores islands (abstract) (children: 0)", @@ -524,16 +531,6 @@ def test_case_insensitive_sort(self) -> None: " azure (ALPHABET) (children: 0)", ] - # And it's case insensitive when getting only a single level: - result = pretty_format_tags(taxonomy.get_filtered_tags(parent_tag_value="ALPHABET", depth=1)) - assert result == [ - " aardvark (ALPHABET) (children: 0)", - " abacus (ALPHABET) (children: 0)", - " Android (ALPHABET) (children: 0)", - " ANVIL (ALPHABET) (children: 0)", - " azure (ALPHABET) (children: 0)", - ] - class TestFilteredTagsFreeTextTaxonomy(TestCase): """ @@ -690,16 +687,13 @@ def test_object_tag_lineage(self): assert object_tag.get_lineage() == ["Another tag"] def test_validate_value_free_text(self): - open_taxonomy = Taxonomy.objects.create( - name="Freetext Life", - allow_free_text=True, - ) + assert self.free_text_taxonomy.allow_free_text # An empty string or other non-string is not valid in a free-text taxonomy - assert open_taxonomy.validate_value("") is False - assert open_taxonomy.validate_value(None) is False - assert open_taxonomy.validate_value(True) is False + assert self.free_text_taxonomy.validate_value("") is False + assert self.free_text_taxonomy.validate_value(None) is False + assert self.free_text_taxonomy.validate_value(True) is False # But any other string value is valid: - assert open_taxonomy.validate_value("Any text we want") is True + assert self.free_text_taxonomy.validate_value("Any text we want") is True def test_validate_value_closed(self): """ @@ -755,8 +749,7 @@ def test_invalid_id(self): """ Test attempting to create object tags with invalid characters in the object ID """ - open_taxonomy = Taxonomy.objects.create(name="Freetext", allow_free_text=True, allow_multiple=True) - args = {"tags": ["test"], "taxonomy": open_taxonomy} + args = {"tags": ["test"], "taxonomy": self.free_text_taxonomy} with pytest.raises(ValidationError): api.tag_object(object_id="wildcard*", **args) with pytest.raises(ValidationError): @@ -766,12 +759,11 @@ def test_invalid_id(self): def test_is_deleted(self): self.taxonomy.allow_multiple = True self.taxonomy.save() - open_taxonomy = Taxonomy.objects.create(name="Freetext Life", allow_free_text=True, allow_multiple=True) object_id = "obj1" # Create some tags: api.tag_object(self.taxonomy, [self.archaea.value, self.bacteria.value], object_id) # Regular tags - api.tag_object(open_taxonomy, ["foo", "bar", "tribble"], object_id) # Free text tags + api.tag_object(self.free_text_taxonomy, ["foo", "bar", "tribble"], object_id) # Free text tags # At first, none of these will be deleted: assert [(t.value, t.is_deleted) for t in api.get_object_tags(object_id)] == [ @@ -794,7 +786,7 @@ def test_is_deleted(self): ] # Then delete the whole free text taxonomy - open_taxonomy.delete() + self.free_text_taxonomy.delete() assert [(t.value, t.is_deleted) for t in api.get_object_tags(object_id)] == [ (self.archaea.value, False), diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index efbd17019..231a7b5b1 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -16,10 +16,11 @@ from openedx_tagging.core.tagging.import_export import api as import_export_api from openedx_tagging.core.tagging.import_export.parsers import ParserFormat from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy -from openedx_tagging.core.tagging.models.system_defined import SystemDefinedTaxonomy, UserSystemDefinedTaxonomy +from openedx_tagging.core.tagging.models.system_defined import SystemDefinedTaxonomy from openedx_tagging.core.tagging.rest_api.paginators import TagsPagination from openedx_tagging.core.tagging.rules import can_change_object_tag_objectid, can_view_object_tag_objectid +from .test_models import TestTagTaxonomyMixin from .utils import pretty_format_tags User = get_user_model() @@ -178,7 +179,7 @@ def test_list_invalid_page(self) -> None: @ddt.unpack def test_detail_taxonomy(self, user_attr: str | None, taxonomy_data: dict[str, bool], expected_status: int): create_data = {"name": "taxonomy detail test", **taxonomy_data} - taxonomy = api.create_taxonomy(**create_data) + taxonomy = api.create_taxonomy(**create_data) # type: ignore[arg-type] url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) if user_attr: @@ -501,12 +502,13 @@ def test_export_taxonomy_unauthorized(self): @ddt.ddt -class TestObjectTagViewSet(APITestCase): +class TestObjectTagViewSet(TestTagTaxonomyMixin, APITestCase): """ Testing various cases for the ObjectTagView. """ def setUp(self): + super().setUp() def _change_object_permission(user, object_id: str) -> bool: """ @@ -526,66 +528,34 @@ def _view_object_permission(user, object_id: str) -> bool: return can_view_object_tag_objectid(user, object_id) - super().setUp() - - self.user = User.objects.create( - username="user", - email="user@example.com", - ) - - self.staff = User.objects.create( - username="staff", - email="staff@example.com", - is_staff=True, - ) - - # System-defined language taxonomy with valid ObjectTag - self.author_sys_taxonomy = api.create_taxonomy(taxonomy_class=UserSystemDefinedTaxonomy, name="Author") - # Tag the object "abc" with the tag for self.staff - api.tag_object(object_id="abc", taxonomy=self.author_sys_taxonomy, tags=[self.staff.username]) - - # Language system-defined language taxonomy - self.language_taxonomy = Taxonomy.objects.get(pk=LANGUAGE_TAXONOMY_ID) - - # Closed Taxonomies created by taxonomy admins, each with 20 ObjectTags - self.enabled_taxonomy = api.create_taxonomy(name="Enabled Taxonomy", allow_multiple=False) - self.disabled_taxonomy = api.create_taxonomy(name="Disabled Taxonomy", enabled=False, allow_multiple=False) - self.multiple_taxonomy = api.create_taxonomy(name="Multiple Taxonomy", allow_multiple=True) - for i in range(20): - api.add_tag_to_taxonomy(taxonomy=self.enabled_taxonomy, tag=f"Tag {i}") - api.add_tag_to_taxonomy(taxonomy=self.disabled_taxonomy, tag=f"Tag {i}") - api.add_tag_to_taxonomy(taxonomy=self.multiple_taxonomy, tag=f"Tag {i}") - api.tag_object(object_id="abc", taxonomy=self.enabled_taxonomy, tags=["Tag 15"]) - api.tag_object(object_id="abc", taxonomy=self.disabled_taxonomy, tags=["Tag 3"]) - api.tag_object(object_id="abc", taxonomy=self.multiple_taxonomy, tags=["Tag 4", "Tag 7", "Tag 12"]) - - # Free-Text Taxonomies created by taxonomy admins, each linked - # to 10 ObjectTags - self.open_taxonomy_enabled = api.create_taxonomy( - name="Enabled Free-Text Taxonomy", allow_free_text=True, allow_multiple=False, - ) - self.open_taxonomy_disabled = api.create_taxonomy( - name="Disabled Free-Text Taxonomy", allow_free_text=True, enabled=False, allow_multiple=False, - ) - for i in range(10): - ObjectTag.objects.create(object_id="abc", taxonomy=self.open_taxonomy_enabled, _value=f"Free Text {i}") - ObjectTag.objects.create(object_id="abc", taxonomy=self.open_taxonomy_disabled, _value=f"Free Text {i}") - # Override the object permission for the test rules.set_perm("oel_tagging.change_objecttag_objectid", _change_object_permission) rules.set_perm("oel_tagging.view_objecttag_objectid", _view_object_permission) + # Create a staff user: + self.staff = User.objects.create(username="staff", email="staff@example.com", is_staff=True) + + # For this test, allow multiple "Life on Earth" tags: + self.taxonomy.allow_multiple = True + self.taxonomy.save() + @ddt.data( - (None, status.HTTP_401_UNAUTHORIZED, None), - ("user", status.HTTP_200_OK, 26), - ("staff", status.HTTP_200_OK, 26), + (None, status.HTTP_401_UNAUTHORIZED), + ("user_1", status.HTTP_200_OK), + ("staff", status.HTTP_200_OK), ) @ddt.unpack - def test_retrieve_object_tags(self, user_attr, expected_status, expected_count): + def test_retrieve_object_tags(self, user_attr, expected_status): """ Test retrieving object tags """ - url = OBJECT_TAGS_RETRIEVE_URL.format(object_id="abc") + object_id = "problem15" + + # Apply the object tags that we're about to retrieve: + api.tag_object(object_id=object_id, taxonomy=self.taxonomy, tags=["Mammalia", "Fungi"]) + api.tag_object(object_id=object_id, taxonomy=self.user_taxonomy, tags=[self.user_1.username]) + + url = OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id) if user_attr: user = getattr(self, user_attr) @@ -595,7 +565,101 @@ def test_retrieve_object_tags(self, user_attr, expected_status, expected_count): assert response.status_code == expected_status if status.is_success(expected_status): - assert len(response.data) == expected_count + # Check the response, first converting from OrderedDict to regular dicts for simplicity. + assert response.data == { + # In the future, this API may allow retrieving tags for multiple objects at once, so it's grouped by + # object ID. + "problem15": { + "taxonomies": [ + { + "name": "Life on Earth", + "taxonomy_id": 1, + "editable": True, + "tags": [ + # Note: based on tree order (Animalia before Fungi), this tag comes first even though it + # starts with "M" and Fungi starts with "F" + { + "value": "Mammalia", + "lineage": ["Eukaryota", "Animalia", "Chordata", "Mammalia"], + }, + { + "value": "Fungi", + "lineage": ["Eukaryota", "Fungi"], + }, + ] + }, + { + "name": "User Authors", + "taxonomy_id": 3, + "editable": False, + "tags": [ + { + "value": "test_user_1", + "lineage": ["test_user_1"], + }, + ], + } + ], + }, + } + + def test_retrieve_object_tags_sorted(self): + """ + Test teh sort order of the object tags retrieved from the get object + tags API. + """ + object_id = "problem7" + + # Apply the object tags that we're about to retrieve: + taxonomy = self.create_sort_test_taxonomy() + api.tag_object(object_id=object_id, taxonomy=taxonomy, tags=[ + "ANVIL", + "Android", + "azores islands", + "abstract", + "11111111", + "111", + "123", + "1 A", + "1111-grandchild", + ]) + # Note: the full taxonomy looks like this, so this is the order we + # expect, although not all of these tags were included. + # 1 + # 1 A + # 11 + # 11111 + # 1111-grandchild + # 2 + # 111 + # 11111111 + # 123 + # abstract + # Andes + # azores islands + # ALPHABET + # aardvark + # abacus + # Android + # ANVIL + # azure + + url = OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id) + self.client.force_authenticate(user=self.user_1) + response = self.client.get(url) + assert response.status_code == 200 + assert response.data[object_id]["taxonomies"][0]["name"] == "Sort Test" + assert response.data[object_id]["taxonomies"][0]["tags"] == [ + {"value": "1 A", "lineage": ["1", "1 A"]}, + {"value": "1111-grandchild", "lineage": ["1", "11111", "1111-grandchild"]}, + {"value": "111", "lineage": ["111"]}, + {"value": "11111111", "lineage": ["111", "11111111"]}, + {"value": "123", "lineage": ["111", "123"]}, + {"value": "abstract", "lineage": ["abstract"]}, + {"value": "azores islands", "lineage": ["abstract", "azores islands"]}, + {"value": "Android", "lineage": ["ALPHABET", "Android"]}, + {"value": "ANVIL", "lineage": ["ALPHABET", "ANVIL"]}, + ] def test_retrieve_object_tags_unauthorized(self): """ @@ -607,34 +671,56 @@ def test_retrieve_object_tags_unauthorized(self): assert response.status_code == status.HTTP_403_FORBIDDEN @ddt.data( - (None, "abc", status.HTTP_401_UNAUTHORIZED, None), - ("user", "abc", status.HTTP_200_OK, 1), - ("staff", "abc", status.HTTP_200_OK, 1), + (None, "abc", status.HTTP_401_UNAUTHORIZED), + ("user_1", "abc", status.HTTP_200_OK), + ("staff", "abc", status.HTTP_200_OK), ) @ddt.unpack def test_retrieve_object_tags_taxonomy_queryparam( - self, user_attr, object_id, expected_status, expected_count + self, user_attr, object_id, expected_status, ): """ Test retrieving object tags for specific taxonomies provided """ + object_id = "html7" + + # Apply the object tags that we're about to retrieve: + api.tag_object(object_id=object_id, taxonomy=self.taxonomy, tags=["Mammalia", "Fungi"]) + api.tag_object(object_id=object_id, taxonomy=self.user_taxonomy, tags=[self.user_1.username]) + url = OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id) if user_attr: user = getattr(self, user_attr) self.client.force_authenticate(user=user) - response = self.client.get(url, {"taxonomy": self.enabled_taxonomy.pk}) + response = self.client.get(url, {"taxonomy": self.user_taxonomy.pk}) assert response.status_code == expected_status if status.is_success(expected_status): - assert len(response.data) == expected_count - for object_tag in response.data: - assert object_tag.get("is_deleted") is False - assert object_tag.get("taxonomy_id") == self.enabled_taxonomy.pk + assert response.data == { + # In the future, this API may allow retrieving tags for multiple objects at once, so it's grouped by + # object ID. + object_id: { + "taxonomies": [ + # The "Life on Earth" tags are excluded here... + { + "name": "User Authors", + "taxonomy_id": 3, + "editable": False, + "tags": [ + { + "value": "test_user_1", + "lineage": ["test_user_1"], + }, + ], + } + ], + }, + } @ddt.data( (None, "abc", status.HTTP_401_UNAUTHORIZED), - ("user", "abc", status.HTTP_400_BAD_REQUEST), + ("user_1", "abc", status.HTTP_400_BAD_REQUEST), ("staff", "abc", status.HTTP_400_BAD_REQUEST), ) @ddt.unpack @@ -656,9 +742,9 @@ def test_retrieve_object_tags_invalid_taxonomy_queryparam(self, user_attr, objec (None, "POST", status.HTTP_401_UNAUTHORIZED), (None, "PATCH", status.HTTP_401_UNAUTHORIZED), (None, "DELETE", status.HTTP_401_UNAUTHORIZED), - ("user", "POST", status.HTTP_405_METHOD_NOT_ALLOWED), - ("user", "PATCH", status.HTTP_405_METHOD_NOT_ALLOWED), - ("user", "DELETE", status.HTTP_405_METHOD_NOT_ALLOWED), + ("user_1", "POST", status.HTTP_405_METHOD_NOT_ALLOWED), + ("user_1", "PATCH", status.HTTP_405_METHOD_NOT_ALLOWED), + ("user_1", "DELETE", status.HTTP_405_METHOD_NOT_ALLOWED), ("staff", "POST", status.HTTP_405_METHOD_NOT_ALLOWED), ("staff", "PATCH", status.HTTP_405_METHOD_NOT_ALLOWED), ("staff", "DELETE", status.HTTP_405_METHOD_NOT_ALLOWED), @@ -694,148 +780,102 @@ def test_object_tags_remaining_http_methods( @ddt.data( # Users and staff can add tags - (None, "language_taxonomy", ["Portuguese"], status.HTTP_401_UNAUTHORIZED), - ("user", "language_taxonomy", ["Portuguese"], status.HTTP_200_OK), - ("staff", "language_taxonomy", ["Portuguese"], status.HTTP_200_OK), - # Users and staff can clear add tags - (None, "enabled_taxonomy", ["Tag 1"], status.HTTP_401_UNAUTHORIZED), - ("user", "enabled_taxonomy", ["Tag 1"], status.HTTP_200_OK), - ("staff", "enabled_taxonomy", ["Tag 1"], status.HTTP_200_OK), + (None, "language_taxonomy", {}, ["Portuguese"], status.HTTP_401_UNAUTHORIZED), + ("user_1", "language_taxonomy", {}, ["Portuguese"], status.HTTP_200_OK), + ("staff", "language_taxonomy", {}, ["Portuguese"], status.HTTP_200_OK), + # user_1s and staff can clear add tags + (None, "taxonomy", {}, ["Fungi"], status.HTTP_401_UNAUTHORIZED), + ("user_1", "taxonomy", {}, ["Fungi"], status.HTTP_200_OK), + ("staff", "taxonomy", {}, ["Fungi"], status.HTTP_200_OK), # Nobody can add tag using a disabled taxonomy - (None, "disabled_taxonomy", ["Tag 1"], status.HTTP_401_UNAUTHORIZED), - ("user", "disabled_taxonomy", ["Tag 1"], status.HTTP_403_FORBIDDEN), - ("staff", "disabled_taxonomy", ["Tag 1"], status.HTTP_403_FORBIDDEN), - # Users and staff can add a single tag using a allow_multiple=True taxonomy - (None, "multiple_taxonomy", ["Tag 1"], status.HTTP_401_UNAUTHORIZED), - ("user", "multiple_taxonomy", ["Tag 1"], status.HTTP_200_OK), - ("staff", "multiple_taxonomy", ["Tag 1"], status.HTTP_200_OK), - # Users and staff can add tags using an open taxonomy - (None, "open_taxonomy_enabled", ["tag1"], status.HTTP_401_UNAUTHORIZED), - ("user", "open_taxonomy_enabled", ["tag1"], status.HTTP_200_OK), - ("staff", "open_taxonomy_enabled", ["tag1"], status.HTTP_200_OK), + (None, "taxonomy", {"enabled": False}, ["Fungi"], status.HTTP_401_UNAUTHORIZED), + ("user_1", "taxonomy", {"enabled": False}, ["Fungi"], status.HTTP_403_FORBIDDEN), + ("staff", "taxonomy", {"enabled": False}, ["Fungi"], status.HTTP_403_FORBIDDEN), + # If allow_multiple=True, multiple tags can be added, but not if it's false: + ("user_1", "taxonomy", {"allow_multiple": True}, ["Mammalia", "Fungi"], status.HTTP_200_OK), + ("user_1", "taxonomy", {"allow_multiple": False}, ["Mammalia", "Fungi"], status.HTTP_400_BAD_REQUEST), + # user_1s and staff can add tags using an open taxonomy + (None, "free_text_taxonomy", {}, ["tag1"], status.HTTP_401_UNAUTHORIZED), + ("user_1", "free_text_taxonomy", {}, ["tag1", "tag2"], status.HTTP_200_OK), + ("staff", "free_text_taxonomy", {}, ["tag1", "tag4"], status.HTTP_200_OK), # Nobody can add tags using a disabled open taxonomy - (None, "open_taxonomy_disabled", ["tag1"], status.HTTP_401_UNAUTHORIZED), - ("user", "open_taxonomy_disabled", ["tag1"], status.HTTP_403_FORBIDDEN), - ("staff", "open_taxonomy_disabled", ["tag1"], status.HTTP_403_FORBIDDEN), + (None, "free_text_taxonomy", {"enabled": False}, ["tag1"], status.HTTP_401_UNAUTHORIZED), + ("user_1", "free_text_taxonomy", {"enabled": False}, ["tag1"], status.HTTP_403_FORBIDDEN), + ("staff", "free_text_taxonomy", {"enabled": False}, ["tag1"], status.HTTP_403_FORBIDDEN), + # Can't add invalid/nonexistent tags using a closed taxonomy + (None, "language_taxonomy", {}, ["Invalid"], status.HTTP_401_UNAUTHORIZED), + ("user_1", "language_taxonomy", {}, ["Invalid"], status.HTTP_400_BAD_REQUEST), + ("staff", "language_taxonomy", {}, ["Invalid"], status.HTTP_400_BAD_REQUEST), + ("staff", "taxonomy", {}, ["Invalid"], status.HTTP_400_BAD_REQUEST), ) @ddt.unpack - def test_tag_object(self, user_attr, taxonomy_attr, tag_values, expected_status): + def test_tag_object(self, user_attr, taxonomy_attr, taxonomy_flags, tag_values, expected_status): if user_attr: user = getattr(self, user_attr) self.client.force_authenticate(user=user) taxonomy = getattr(self, taxonomy_attr) + if taxonomy_flags: + for (k, v) in taxonomy_flags.items(): + setattr(taxonomy, k, v) + taxonomy.save() - url = OBJECT_TAGS_UPDATE_URL.format(object_id="abc", taxonomy_id=taxonomy.pk) + object_id = "abc" - response = self.client.put(url, {"tags": tag_values}, format="json") - assert response.status_code == expected_status - if status.is_success(expected_status): - assert len(response.data) == len(tag_values) - assert set(t["value"] for t in response.data) == set(tag_values) - - @ddt.data( - # Can't add invalid tags using a closed taxonomy - (None, "language_taxonomy", ["Invalid"], status.HTTP_401_UNAUTHORIZED), - ("user", "language_taxonomy", ["Invalid"], status.HTTP_400_BAD_REQUEST), - ("staff", "language_taxonomy", ["Invalid"], status.HTTP_400_BAD_REQUEST), - (None, "enabled_taxonomy", ["invalid"], status.HTTP_401_UNAUTHORIZED), - ("user", "enabled_taxonomy", ["invalid"], status.HTTP_400_BAD_REQUEST), - ("staff", "enabled_taxonomy", ["invalid"], status.HTTP_400_BAD_REQUEST), - (None, "multiple_taxonomy", ["invalid"], status.HTTP_401_UNAUTHORIZED), - ("user", "multiple_taxonomy", ["invalid"], status.HTTP_400_BAD_REQUEST), - ("staff", "multiple_taxonomy", ["invalid"], status.HTTP_400_BAD_REQUEST), - # Nobody can edit object tags using a disabled taxonomy. - (None, "disabled_taxonomy", ["invalid"], status.HTTP_401_UNAUTHORIZED), - ("user", "disabled_taxonomy", ["invalid"], status.HTTP_403_FORBIDDEN), - ("staff", "disabled_taxonomy", ["invalid"], status.HTTP_403_FORBIDDEN), - ) - @ddt.unpack - def test_tag_object_invalid(self, user_attr, taxonomy_attr, tag_values, expected_status): - if user_attr: - user = getattr(self, user_attr) - self.client.force_authenticate(user=user) - - taxonomy = getattr(self, taxonomy_attr) - - url = OBJECT_TAGS_UPDATE_URL.format(object_id="abc", taxonomy_id=taxonomy.pk) + url = OBJECT_TAGS_UPDATE_URL.format(object_id=object_id, taxonomy_id=taxonomy.pk) response = self.client.put(url, {"tags": tag_values}, format="json") assert response.status_code == expected_status - assert not status.is_success(expected_status) # No success cases here + if status.is_success(expected_status): + assert [t["value"] for t in response.data[object_id]["taxonomies"][0]["tags"]] == tag_values + # And retrieving the object tags again should return an identical response: + assert response.data == self.client.get(OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id)).data @ddt.data( # Users and staff can clear tags - (None, "enabled_taxonomy", [], status.HTTP_401_UNAUTHORIZED), - ("user", "enabled_taxonomy", [], status.HTTP_200_OK), - ("staff", "enabled_taxonomy", [], status.HTTP_200_OK), - # Users and staff can clear object tags using a allow_multiple=True taxonomy - (None, "multiple_taxonomy", [], status.HTTP_401_UNAUTHORIZED), - ("user", "multiple_taxonomy", [], status.HTTP_200_OK), - ("staff", "multiple_taxonomy", [], status.HTTP_200_OK), + (None, {}, status.HTTP_401_UNAUTHORIZED), + ("user_1", {}, status.HTTP_200_OK), + ("staff", {}, status.HTTP_200_OK), # Nobody can clear tags using a disabled taxonomy - (None, "disabled_taxonomy", [], status.HTTP_401_UNAUTHORIZED), - ("user", "disabled_taxonomy", [], status.HTTP_403_FORBIDDEN), - ("staff", "disabled_taxonomy", [], status.HTTP_403_FORBIDDEN), - (None, "open_taxonomy_disabled", [], status.HTTP_401_UNAUTHORIZED), - ("user", "open_taxonomy_disabled", [], status.HTTP_403_FORBIDDEN), - ("staff", "open_taxonomy_disabled", [], status.HTTP_403_FORBIDDEN), + (None, {"enabled": False}, status.HTTP_401_UNAUTHORIZED), + ("user_1", {"enabled": False}, status.HTTP_403_FORBIDDEN), + ("staff", {"enabled": False}, status.HTTP_403_FORBIDDEN), + # ... and it doesn't matter if it's free text or closed: + ("staff", {"enabled": False, "allow_free_text": False}, status.HTTP_403_FORBIDDEN), ) @ddt.unpack - def test_tag_object_clear(self, user_attr, taxonomy_attr, tag_values, expected_status): - if user_attr: - user = getattr(self, user_attr) - self.client.force_authenticate(user=user) - - taxonomy = getattr(self, taxonomy_attr) - - url = OBJECT_TAGS_UPDATE_URL.format(object_id="abc", taxonomy_id=taxonomy.pk) + def test_tag_object_clear(self, user_attr, taxonomy_flags, expected_status): + """ + Test that authorized users can *remove* tags using this API + """ + object_id = "abc" - response = self.client.put(url, {"tags": tag_values}, format="json") - assert response.status_code == expected_status - if status.is_success(expected_status): - assert len(response.data) == len(tag_values) - assert set(t["value"] for t in response.data) == set(tag_values) + # First create an object tag: + api.tag_object(object_id=object_id, taxonomy=self.taxonomy, tags=["Fungi"]) - @ddt.data( - # Users and staff can add multiple tags using a allow_multiple=True taxonomy - (None, "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_401_UNAUTHORIZED), - ("user", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), - ("staff", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), - (None, "open_taxonomy_enabled", ["tag1", "tag2"], status.HTTP_401_UNAUTHORIZED), - ("user", "open_taxonomy_enabled", ["tag1", "tag2"], status.HTTP_400_BAD_REQUEST), - ("staff", "open_taxonomy_enabled", ["tag1", "tag2"], status.HTTP_400_BAD_REQUEST), - # Users and staff can't add multple tags using a allow_multiple=False taxonomy - (None, "enabled_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_401_UNAUTHORIZED), - ("user", "enabled_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_400_BAD_REQUEST), - ("staff", "enabled_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_400_BAD_REQUEST), - (None, "language_taxonomy", ["Portuguese", "English"], status.HTTP_401_UNAUTHORIZED), - ("user", "language_taxonomy", ["Portuguese", "English"], status.HTTP_400_BAD_REQUEST), - ("staff", "language_taxonomy", ["Portuguese", "English"], status.HTTP_400_BAD_REQUEST), - # Nobody can edit tags using a disabled taxonomy. - (None, "disabled_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_401_UNAUTHORIZED), - ("user", "disabled_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_403_FORBIDDEN), - ("staff", "disabled_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_403_FORBIDDEN), - ) - @ddt.unpack - def test_tag_object_multiple(self, user_attr, taxonomy_attr, tag_values, expected_status): if user_attr: user = getattr(self, user_attr) self.client.force_authenticate(user=user) - taxonomy = getattr(self, taxonomy_attr) + if taxonomy_flags: + for (k, v) in taxonomy_flags.items(): + setattr(self.taxonomy, k, v) + self.taxonomy.save() - url = OBJECT_TAGS_UPDATE_URL.format(object_id="abc", taxonomy_id=taxonomy.pk) + url = OBJECT_TAGS_UPDATE_URL.format(object_id=object_id, taxonomy_id=self.taxonomy.pk) - response = self.client.put(url, {"tags": tag_values}, format="json") + response = self.client.put(url, {"tags": []}, format="json") assert response.status_code == expected_status if status.is_success(expected_status): - assert len(response.data) == len(tag_values) - assert set(t["value"] for t in response.data) == set(tag_values) + # Now there are no tags applied: + assert response.data[object_id]["taxonomies"] == [] + else: + # Make sure the object tags are unchanged: + assert [ot.value for ot in api.get_object_tags(object_id=object_id)] == ["Fungi"] @ddt.data( (None, status.HTTP_401_UNAUTHORIZED), - ("user", status.HTTP_403_FORBIDDEN), + ("user_1", status.HTTP_403_FORBIDDEN), ("staff", status.HTTP_403_FORBIDDEN), ) @ddt.unpack @@ -844,7 +884,7 @@ def test_tag_object_without_permission(self, user_attr, expected_status): user = getattr(self, user_attr) self.client.force_authenticate(user=user) - url = OBJECT_TAGS_UPDATE_URL.format(object_id="view_only", taxonomy_id=self.enabled_taxonomy.pk) + url = OBJECT_TAGS_UPDATE_URL.format(object_id="view_only", taxonomy_id=self.taxonomy.pk) response = self.client.put(url, {"tags": ["Tag 1"]}, format="json") assert response.status_code == expected_status @@ -855,13 +895,9 @@ def test_tag_object_count_limit(self): Checks if the limit of 100 tags per object is enforced """ object_id = "limit_tag_count" - dummy_taxonomies = [] - for i in range(100): - taxonomy = api.create_taxonomy(name=f"Dummy Taxonomy {i}", allow_free_text=True, allow_multiple=True) - api.tag_object(object_id=object_id, taxonomy=taxonomy, tags=["Dummy Tag"]) - dummy_taxonomies.append(taxonomy) + dummy_taxonomies = self.create_100_taxonomies() - url = OBJECT_TAGS_UPDATE_URL.format(object_id=object_id, taxonomy_id=self.enabled_taxonomy.pk) + url = OBJECT_TAGS_UPDATE_URL.format(object_id=object_id, taxonomy_id=self.taxonomy.pk) self.client.force_authenticate(user=self.staff) response = self.client.put(url, {"tags": ["Tag 1"]}, format="json") # Can't add another tag because the object already has 100 tags From 2858e1be07f2e42cd300f899200bfe9352d06c89 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 1 Nov 2023 14:32:01 -0700 Subject: [PATCH 08/14] feat: Add an API to retrieve the count of tags applied to each object --- .../core/tagging/rest_api/v1/urls.py | 1 + .../core/tagging/rest_api/v1/views.py | 54 ++++++++++++++++-- .../core/tagging/test_views.py | 56 +++++++++++++++++++ 3 files changed, 106 insertions(+), 5 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/urls.py b/openedx_tagging/core/tagging/rest_api/v1/urls.py index 72ff87d0d..b7841b574 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/urls.py +++ b/openedx_tagging/core/tagging/rest_api/v1/urls.py @@ -10,6 +10,7 @@ router = DefaultRouter() router.register("taxonomies", views.TaxonomyView, basename="taxonomy") router.register("object_tags", views.ObjectTagView, basename="object_tag") +router.register("object_tag_counts", views.ObjectTagCountsView, basename="object_tag_counts") urlpatterns = [ path("", include(router.urls)), diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index 7b6200f8c..16798fa1f 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -3,6 +3,8 @@ """ from __future__ import annotations +from typing import Any + from django.db import models from django.http import Http404, HttpResponse from rest_framework import mixins, status @@ -26,7 +28,7 @@ from ...data import TagDataQuerySet from ...import_export.api import export_tags from ...import_export.parsers import ParserFormat -from ...models import Taxonomy +from ...models import ObjectTag, Taxonomy from ...rules import ObjectTagPermissionItem from ..paginators import TAGS_THRESHOLD, DisabledTagsPagination, TagsPagination from .permissions import ObjectTagObjectPermissions, TagObjectPermissions, TaxonomyObjectPermissions @@ -266,10 +268,6 @@ class ObjectTagView( * 400 - Invalid query parameter * 403 - Permission denied - **Create Query Returns** - * 403 - Permission denied - * 405 - Method not allowed - **Update Parameters** * object_id (required): - The Object ID to add ObjectTags for. @@ -411,6 +409,52 @@ def update(self, request, *args, **kwargs) -> Response: return self.retrieve(request, object_id) +@view_auth_classes +class ObjectTagCountsView( + mixins.RetrieveModelMixin, + GenericViewSet, +): + """ + View to retrieve the count of ObjectTags for all matching object IDs. + + This API does NOT bother doing any permission checks as the "# of tags" is not considered sensitive information. + + **Retrieve Parameters** + * object_id_pattern (required): - The Object ID to retrieve ObjectTags for. Can contain '*' at the end + for wildcard matching, or use ',' to separate multiple object IDs. + + **Retrieve Example Requests** + GET api/tagging/v1/object_tag_counts/:object_id_pattern + + **Retrieve Query Returns** + * 200 - Success + """ + + serializer_class = ObjectTagSerializer + lookup_field = "object_id_pattern" + + def retrieve(self, request, *args, **kwargs) -> Response: + """ + Retrieve the counts of object tags that belong to a given object_id pattern + + Note: We override `retrieve` here instead of `list` because we are + passing in the Object ID (object_id) in the path (as opposed to passing + it in as a query_param) to retrieve the ObjectTag counts. + """ + # This API does NOT bother doing any permission checks as the # of tags is not considered sensitive information. + object_id_pattern = self.kwargs["object_id_pattern"] + qs: Any = ObjectTag.objects + if object_id_pattern.endswith("*"): + qs = qs.filter(object_id__startswith=object_id_pattern[0:len(object_id_pattern) - 1]) + elif "*" in object_id_pattern: + raise ValidationError("Wildcard matches are only supported if the * is at the end.") + else: + qs = qs.filter(object_id__in=object_id_pattern.split(",")) + + qs = qs.values("object_id").annotate(num_tags=models.Count("id")).order_by("object_id") + return Response({row["object_id"]: row["num_tags"] for row in qs}) + + @view_auth_classes class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): """ diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 231a7b5b1..cd0b303b6 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -32,6 +32,7 @@ OBJECT_TAGS_RETRIEVE_URL = "/tagging/rest_api/v1/object_tags/{object_id}/" +OBJECT_TAG_COUNTS_URL = "/tagging/rest_api/v1/object_tag_counts/{object_id_pattern}/" OBJECT_TAGS_UPDATE_URL = "/tagging/rest_api/v1/object_tags/{object_id}/?taxonomy={taxonomy_id}" LANGUAGE_TAXONOMY_ID = -1 @@ -916,6 +917,61 @@ def test_tag_object_count_limit(self): assert response.status_code == status.HTTP_400_BAD_REQUEST +class TestObjectTagCountsViewSet(TestTagTaxonomyMixin, APITestCase): + """ + Testing various cases for counting how many tags are applied to several objects. + """ + + def test_get_counts(self): + """ + Test retrieving the counts of tags applied to various content objects. + + This API does NOT bother doing any permission checks as the "# of tags" is not considered sensitive information. + """ + # Course 2 + api.tag_object(object_id="course02-unit01-problem01", taxonomy=self.free_text_taxonomy, tags=["other"]) + # Course 7 Unit 1 + api.tag_object(object_id="course07-unit01-problem01", taxonomy=self.free_text_taxonomy, tags=["a", "b", "c"]) + api.tag_object(object_id="course07-unit01-problem02", taxonomy=self.free_text_taxonomy, tags=["a", "b"]) + # Course 7 Unit 2 + api.tag_object(object_id="course07-unit02-problem01", taxonomy=self.free_text_taxonomy, tags=["b"]) + api.tag_object(object_id="course07-unit02-problem02", taxonomy=self.free_text_taxonomy, tags=["c", "d"]) + api.tag_object(object_id="course07-unit02-problem03", taxonomy=self.free_text_taxonomy, tags=["N", "M", "x"]) + + def check(object_id_pattern: str): + result = self.client.get(OBJECT_TAG_COUNTS_URL.format(object_id_pattern=object_id_pattern)) + assert result.status_code == status.HTTP_200_OK + return result.data + + with self.assertNumQueries(1): + assert check(object_id_pattern="course02-*") == { + "course02-unit01-problem01": 1, + } + with self.assertNumQueries(1): + assert check(object_id_pattern="course07-unit01-*") == { + "course07-unit01-problem01": 3, + "course07-unit01-problem02": 2, + } + with self.assertNumQueries(1): + assert check(object_id_pattern="course07-unit*") == { + "course07-unit01-problem01": 3, + "course07-unit01-problem02": 2, + "course07-unit02-problem01": 1, + "course07-unit02-problem02": 2, + "course07-unit02-problem03": 3, + } + # Can also use a comma to separate explicit object IDs: + with self.assertNumQueries(1): + assert check(object_id_pattern="course07-unit01-problem01") == { + "course07-unit01-problem01": 3, + } + with self.assertNumQueries(1): + assert check(object_id_pattern="course07-unit01-problem01,course07-unit02-problem02") == { + "course07-unit01-problem01": 3, + "course07-unit02-problem02": 2, + } + + class TestTaxonomyTagsView(TestTaxonomyViewMixin): """ Tests the list/create/update/delete tags of taxonomy view From f974645ec4a056e7ef0c7f683c1e111540ed07e9 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 1 Nov 2023 15:02:35 -0700 Subject: [PATCH 09/14] fix: get_object_tags() should sort first by taxonomy, then tag tree value --- openedx_tagging/core/tagging/api.py | 3 ++- .../core/tagging/rest_api/v1/views.py | 2 +- tests/openedx_tagging/core/tagging/test_api.py | 12 ++++++------ .../core/tagging/test_models.py | 18 +++++++++--------- 4 files changed, 18 insertions(+), 17 deletions(-) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index f0f23eb54..266491e4d 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -176,7 +176,8 @@ def get_object_tags( Value("\t"), output_field=models.CharField(), ))) - .order_by("sort_key") + .annotate(taxonomy_name=Coalesce(F("taxonomy__name"), F("_name"))) + .order_by("taxonomy_name", "sort_key") ) return tags diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index 16798fa1f..d2566ca2c 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -333,7 +333,7 @@ def retrieve(self, request, *args, **kwargs) -> Response: path and returns a it as a single result however that is not behavior we want. """ - object_tags = self.filter_queryset(self.get_queryset()).order_by("sort_key") + object_tags = self.filter_queryset(self.get_queryset()) serializer = ObjectTagsByTaxonomySerializer(list(object_tags)) response_data = serializer.data if self.kwargs["object_id"] not in response_data: diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index a488cc824..a5d6f9366 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -266,20 +266,20 @@ def test_resync_object_tags(self) -> None: # At first, none of these will be deleted: assert [(t.value, t.is_deleted) for t in tagging_api.get_object_tags(object_id)] == [ - (self.archaea.value, False), - (self.bacteria.value, False), ("bar", False), ("foo", False), + (self.archaea.value, False), + (self.bacteria.value, False), ] # Delete "bacteria" from the taxonomy: tagging_api.delete_tags_from_taxonomy(self.taxonomy, [self.bacteria.value], with_subtags=True) assert [(t.value, t.is_deleted) for t in tagging_api.get_object_tags(object_id)] == [ - (self.archaea.value, False), - (self.bacteria.value, True), # <--- deleted! But the value is preserved. ("bar", False), ("foo", False), + (self.archaea.value, False), + (self.bacteria.value, True), # <--- deleted! But the value is preserved. ] # Re-syncing the tags at this point does nothing: @@ -294,10 +294,10 @@ def test_resync_object_tags(self) -> None: # Now the tag is not deleted: assert [(t.value, t.is_deleted) for t in tagging_api.get_object_tags(object_id)] == [ - (self.archaea.value, False), - (self.bacteria.value, False), # <--- not deleted ("bar", False), ("foo", False), + (self.archaea.value, False), + (self.bacteria.value, False), # <--- not deleted ] # Re-syncing the tags now does nothing: diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index 7d4698519..88bfeaad2 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -767,31 +767,31 @@ def test_is_deleted(self): # At first, none of these will be deleted: assert [(t.value, t.is_deleted) for t in api.get_object_tags(object_id)] == [ - (self.archaea.value, False), - (self.bacteria.value, False), - ("foo", False), ("bar", False), + ("foo", False), ("tribble", False), + (self.archaea.value, False), + (self.bacteria.value, False), ] # Delete "bacteria" from the taxonomy: api.delete_tags_from_taxonomy(self.taxonomy, ["Bacteria"], with_subtags=True) assert [(t.value, t.is_deleted) for t in api.get_object_tags(object_id)] == [ - (self.archaea.value, False), - (self.bacteria.value, True), # <--- deleted! But the value is preserved. - ("foo", False), ("bar", False), + ("foo", False), ("tribble", False), + (self.archaea.value, False), + (self.bacteria.value, True), # <--- deleted! But the value is preserved. ] # Then delete the whole free text taxonomy self.free_text_taxonomy.delete() assert [(t.value, t.is_deleted) for t in api.get_object_tags(object_id)] == [ - (self.archaea.value, False), - (self.bacteria.value, True), # <--- deleted! But the value is preserved. - ("foo", True), # <--- Deleted, but the value is preserved ("bar", True), # <--- Deleted, but the value is preserved + ("foo", True), # <--- Deleted, but the value is preserved ("tribble", True), # <--- Deleted, but the value is preserved + (self.archaea.value, False), + (self.bacteria.value, True), # <--- deleted! But the value is preserved. ] From 984c042a7fa4b073ba429018fe2e66ecec1c9ab0 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 1 Nov 2023 15:07:35 -0700 Subject: [PATCH 10/14] docs: add some comments to explain the query we're using --- openedx_tagging/core/tagging/api.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index 266491e4d..8c5ed7648 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -167,7 +167,9 @@ def get_object_tags( filters = {"taxonomy_id": taxonomy_id} if taxonomy_id else {} tags = ( object_tag_class.objects.filter(object_id=object_id, **filters) + # Preload related objects, including data for the "get_lineage" method on ObjectTag/Tag: .select_related("taxonomy", "tag", "tag__parent", "tag__parent__parent") + # Sort the tags within each taxonomy in "tree order". See Taxonomy._get_filtered_tags_deep for details on this: .annotate(sort_key=Lower(Concat( ConcatNull(F("tag__parent__parent__parent__value"), Value("\t")), ConcatNull(F("tag__parent__parent__value"), Value("\t")), @@ -177,6 +179,7 @@ def get_object_tags( output_field=models.CharField(), ))) .annotate(taxonomy_name=Coalesce(F("taxonomy__name"), F("_name"))) + # Sort first by taxonomy name, then by tag value in tree order: .order_by("taxonomy_name", "sort_key") ) return tags From 454acbb1540649b85400f5b32ebe77c15de0aaae Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 1 Nov 2023 15:18:10 -0700 Subject: [PATCH 11/14] fix: fix inconsistent database ID hard coded in test --- tests/openedx_tagging/core/tagging/test_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index a5d6f9366..a8c2e7569 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -93,7 +93,7 @@ def test_get_taxonomies(self) -> None: self.user_taxonomy, ] assert str(enabled[0]) == f" ({tax1.id}) Enabled" - assert str(enabled[1]) == " (6) Free Text" + assert str(enabled[1]) == f" ({self.free_text_taxonomy.id}) Free Text" assert str(enabled[2]) == " (5) Import Taxonomy Test" assert str(enabled[3]) == " (-1) Languages" assert str(enabled[4]) == " (1) Life on Earth" From 562b6df879ad6c309d2671df3e4e3ae1b64bf6ac Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 1 Nov 2023 15:18:24 -0700 Subject: [PATCH 12/14] feat: ensure get_object_tags uses only one database query --- .../core/tagging/test_views.py | 57 +++++++++++++------ 1 file changed, 41 insertions(+), 16 deletions(-) diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index cd0b303b6..339ef017d 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -604,16 +604,13 @@ def test_retrieve_object_tags(self, user_attr, expected_status): }, } - def test_retrieve_object_tags_sorted(self): + def prepare_for_sort_test(self) -> tuple[str, list[str]]: """ - Test teh sort order of the object tags retrieved from the get object - tags API. + Tag an object with tags from the "sort test" taxonomy """ object_id = "problem7" - - # Apply the object tags that we're about to retrieve: - taxonomy = self.create_sort_test_taxonomy() - api.tag_object(object_id=object_id, taxonomy=taxonomy, tags=[ + # Some selected tags to use, from the taxonomy create by self.create_sort_test_taxonomy() + sort_test_tags = [ "ANVIL", "Android", "azores islands", @@ -623,8 +620,14 @@ def test_retrieve_object_tags_sorted(self): "123", "1 A", "1111-grandchild", - ]) - # Note: the full taxonomy looks like this, so this is the order we + ] + + # Apply the object tags: + taxonomy = self.create_sort_test_taxonomy() + api.tag_object(object_id=object_id, taxonomy=taxonomy, tags=sort_test_tags) + + # The result we expect to see when retrieving the object tags, after applying the list above. + # Note: the full taxonomy looks like the following, so this is the order we # expect, although not all of these tags were included. # 1 # 1 A @@ -644,13 +647,7 @@ def test_retrieve_object_tags_sorted(self): # Android # ANVIL # azure - - url = OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id) - self.client.force_authenticate(user=self.user_1) - response = self.client.get(url) - assert response.status_code == 200 - assert response.data[object_id]["taxonomies"][0]["name"] == "Sort Test" - assert response.data[object_id]["taxonomies"][0]["tags"] == [ + sort_test_applied_result = [ {"value": "1 A", "lineage": ["1", "1 A"]}, {"value": "1111-grandchild", "lineage": ["1", "11111", "1111-grandchild"]}, {"value": "111", "lineage": ["111"]}, @@ -661,6 +658,34 @@ def test_retrieve_object_tags_sorted(self): {"value": "Android", "lineage": ["ALPHABET", "Android"]}, {"value": "ANVIL", "lineage": ["ALPHABET", "ANVIL"]}, ] + return object_id, sort_test_applied_result + + def test_retrieve_object_tags_sorted(self): + """ + Test teh sort order of the object tags retrieved from the get object + tags API. + """ + object_id, sort_test_applied_result = self.prepare_for_sort_test() + + url = OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id) + self.client.force_authenticate(user=self.user_1) + response = self.client.get(url) + assert response.status_code == 200 + assert response.data[object_id]["taxonomies"][0]["name"] == "Sort Test" + assert response.data[object_id]["taxonomies"][0]["tags"] == sort_test_applied_result + + def test_retrieve_object_tags_query_count(self): + """ + Test how many queries are used when retrieving object tags + """ + object_id, sort_test_applied_result = self.prepare_for_sort_test() + + url = OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id) + self.client.force_authenticate(user=self.user_1) + with self.assertNumQueries(1): + response = self.client.get(url) + assert response.status_code == 200 + assert response.data[object_id]["taxonomies"][0]["tags"] == sort_test_applied_result def test_retrieve_object_tags_unauthorized(self): """ From b2c8d76bc4aa9faa5a94d76b8bdfe7d416be9ffe Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 2 Nov 2023 10:32:11 -0700 Subject: [PATCH 13/14] fix: address issues found in review --- .../core/tagging/rest_api/v1/serializers.py | 3 +- .../core/tagging/test_views.py | 28 +++++++++---------- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index a8fb7a94e..3281af3f0 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -72,7 +72,7 @@ class Meta: model = ObjectTag fields = ["value", "lineage"] - lineage = serializers.ListField(child=serializers.CharField(), source="get_lineage") + lineage = serializers.ListField(child=serializers.CharField(), source="get_lineage", read_only=True) class ObjectTagSerializer(ObjectTagMinimalSerializer): @@ -80,6 +80,7 @@ class ObjectTagSerializer(ObjectTagMinimalSerializer): Serializer for the ObjectTag model. """ class Meta: + model = ObjectTag fields = ObjectTagMinimalSerializer.Meta.fields + [ # The taxonomy name "name", diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 339ef017d..bdb214aed 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -103,9 +103,9 @@ class TestTaxonomyViewSet(TestTaxonomyViewMixin): ) @ddt.unpack def test_list_taxonomy_queryparams(self, enabled, expected_status: int, expected_count: int | None): - Taxonomy.objects.create(name="Taxonomy enabled 1", enabled=True).save() - Taxonomy.objects.create(name="Taxonomy enabled 2", enabled=True).save() - Taxonomy.objects.create(name="Taxonomy disabled", enabled=False).save() + api.create_taxonomy(name="Taxonomy enabled 1", enabled=True) + api.create_taxonomy(name="Taxonomy enabled 2", enabled=True) + api.create_taxonomy(name="Taxonomy disabled", enabled=False) url = TAXONOMY_LIST_URL @@ -139,11 +139,11 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int): def test_list_taxonomy_pagination(self) -> None: url = TAXONOMY_LIST_URL - api.create_taxonomy(name="T1", enabled=True).save() - api.create_taxonomy(name="T2", enabled=True).save() - api.create_taxonomy(name="T3", enabled=False).save() - api.create_taxonomy(name="T4", enabled=False).save() - api.create_taxonomy(name="T5", enabled=False).save() + api.create_taxonomy(name="T1", enabled=True) + api.create_taxonomy(name="T2", enabled=True) + api.create_taxonomy(name="T3", enabled=False) + api.create_taxonomy(name="T4", enabled=False) + api.create_taxonomy(name="T5", enabled=False) self.client.force_authenticate(user=self.staff) @@ -604,7 +604,7 @@ def test_retrieve_object_tags(self, user_attr, expected_status): }, } - def prepare_for_sort_test(self) -> tuple[str, list[str]]: + def prepare_for_sort_test(self) -> tuple[str, list[dict]]: """ Tag an object with tags from the "sort test" taxonomy """ @@ -662,7 +662,7 @@ def prepare_for_sort_test(self) -> tuple[str, list[str]]: def test_retrieve_object_tags_sorted(self): """ - Test teh sort order of the object tags retrieved from the get object + Test the sort order of the object tags retrieved from the get object tags API. """ object_id, sort_test_applied_result = self.prepare_for_sort_test() @@ -697,13 +697,13 @@ def test_retrieve_object_tags_unauthorized(self): assert response.status_code == status.HTTP_403_FORBIDDEN @ddt.data( - (None, "abc", status.HTTP_401_UNAUTHORIZED), - ("user_1", "abc", status.HTTP_200_OK), - ("staff", "abc", status.HTTP_200_OK), + (None, status.HTTP_401_UNAUTHORIZED), + ("user_1", status.HTTP_200_OK), + ("staff", status.HTTP_200_OK), ) @ddt.unpack def test_retrieve_object_tags_taxonomy_queryparam( - self, user_attr, object_id, expected_status, + self, user_attr, expected_status, ): """ Test retrieving object tags for specific taxonomies provided From 2309a0b3eaeb3d00d191ae0634bcb3282243ec76 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 2 Nov 2023 11:13:55 -0700 Subject: [PATCH 14/14] chore: version bump --- openedx_learning/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index 38d376f15..711ab06c2 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -1,4 +1,4 @@ """ Open edX Learning ("Learning Core"). """ -__version__ = "0.3.1" +__version__ = "0.3.2"