From bad29ee174d5146197b78b564ee31f34436bedf0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 19 Sep 2023 11:55:36 -0300 Subject: [PATCH 01/13] feat: remove pagination from ObjectTag viewset --- .../core/tagging/rest_api/v1/views.py | 18 ++-- .../core/tagging/test_views.py | 98 +++++-------------- 2 files changed, 32 insertions(+), 84 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index 008859f5a..64b7b743d 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -5,6 +5,7 @@ from django.http import Http404 from rest_framework import mixins from rest_framework.exceptions import MethodNotAllowed, PermissionDenied, ValidationError +from rest_framework.response import Response from rest_framework.viewsets import GenericViewSet, ModelViewSet from ...api import create_taxonomy, get_object_tags, get_taxonomies, get_taxonomy, tag_object @@ -171,21 +172,17 @@ def perform_create(self, serializer) -> None: class ObjectTagView(mixins.RetrieveModelMixin, mixins.UpdateModelMixin, mixins.ListModelMixin, GenericViewSet): """ - View to retrieve paginated ObjectTags for a provided Object ID (object_id). + View to retrieve ObjectTags for a provided Object ID (object_id). **Retrieve Parameters** * object_id (required): - The Object ID to retrieve ObjectTags for. **Retrieve Query Parameters** * taxonomy (optional) - PK of taxonomy to filter ObjectTags for. - * page (optional) - Page number of paginated results. - * page_size (optional) - Number of results included in each page. **Retrieve Example Requests** GET api/tagging/v1/object_tags/:object_id GET api/tagging/v1/object_tags/:object_id?taxonomy=1 - GET api/tagging/v1/object_tags/:object_id?taxonomy=1&page=2 - GET api/tagging/v1/object_tags/:object_id?taxonomy=1&page=2&page_size=10 **Retrieve Query Returns** * 200 - Success @@ -232,8 +229,7 @@ def get_queryset(self) -> models.QuerySet: def retrieve(self, request, object_id=None): """ - Retrieve ObjectTags that belong to a given object_id and - return paginated results. + Retrieve ObjectTags that belong to a given object_id 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 @@ -243,14 +239,12 @@ def retrieve(self, request, object_id=None): behavior we want. """ object_tags = self.get_queryset() - paginated_object_tags = self.paginate_queryset(object_tags) - serializer = ObjectTagSerializer(paginated_object_tags, many=True) - return self.get_paginated_response(serializer.data) + serializer = ObjectTagSerializer(object_tags, many=True) + return Response(serializer.data) def update(self, request, object_id, partial=False): """ - Update ObjectTags that belong to a given object_id and - return the list of these ObjecTags paginated. + Update ObjectTags that belong to a given object_id Pass a list of Tag ids or Tag values to be applied to an object id in the body `tag` parameter. Passing an empty list will remove all tags from diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index a50ac3e65..e698a1216 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -449,12 +449,12 @@ def _object_permission(_user, object_id: str) -> bool: ) # Free-Text Taxonomies created by taxonomy admins, each linked - # to 200 ObjectTags + # to 10 ObjectTags self.open_taxonomy_enabled = Taxonomy.objects.create(name="Enabled Free-Text Taxonomy", allow_free_text=True) self.open_taxonomy_disabled = Taxonomy.objects.create( name="Disabled Free-Text Taxonomy", allow_free_text=True, enabled=False ) - for i in range(200): + 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}") @@ -462,15 +462,15 @@ def _object_permission(_user, object_id: str) -> bool: rules.set_perm("oel_tagging.change_objecttag_objectid", _object_permission) @ddt.data( - (None, "abc", status.HTTP_403_FORBIDDEN, None, None), - ("user", "abc", status.HTTP_200_OK, 461, 10), - ("staff", "abc", status.HTTP_200_OK, 461, 10), - (None, "non-existing-id", status.HTTP_403_FORBIDDEN, None, None), - ("user", "non-existing-id", status.HTTP_200_OK, 0, 0), - ("staff", "non-existing-id", status.HTTP_200_OK, 0, 0), + (None, "abc", status.HTTP_403_FORBIDDEN, None), + ("user", "abc", status.HTTP_200_OK, 81), + ("staff", "abc", status.HTTP_200_OK, 81), + (None, "non-existing-id", status.HTTP_403_FORBIDDEN, None), + ("user", "non-existing-id", status.HTTP_200_OK, 0), + ("staff", "non-existing-id", status.HTTP_200_OK, 0), ) @ddt.unpack - def test_retrieve_object_tags(self, user_attr, object_id, expected_status, expected_count, expected_results): + def test_retrieve_object_tags(self, user_attr, object_id, expected_status, expected_count): """ Test retrieving object tags """ @@ -484,18 +484,16 @@ def test_retrieve_object_tags(self, user_attr, object_id, expected_status, expec assert response.status_code == expected_status if status.is_success(expected_status): - assert response.data.get("count") == expected_count - assert response.data.get("results") is not None - assert len(response.data.get("results")) == expected_results + assert len(response.data) == expected_count @ddt.data( - (None, "abc", status.HTTP_403_FORBIDDEN, None, None), - ("user", "abc", status.HTTP_200_OK, 20, 10), - ("staff", "abc", status.HTTP_200_OK, 20, 10), + (None, "abc", status.HTTP_403_FORBIDDEN, None), + ("user", "abc", status.HTTP_200_OK, 20), + ("staff", "abc", status.HTTP_200_OK, 20), ) @ddt.unpack def test_retrieve_object_tags_taxonomy_queryparam( - self, user_attr, object_id, expected_status, expected_count, expected_results + self, user_attr, object_id, expected_status, expected_count ): """ Test retrieving object tags for specific taxonomies provided @@ -509,11 +507,8 @@ def test_retrieve_object_tags_taxonomy_queryparam( response = self.client.get(url, {"taxonomy": self.enabled_taxonomy.pk}) assert response.status_code == expected_status if status.is_success(expected_status): - assert response.data.get("count") == expected_count - assert response.data.get("results") is not None - assert len(response.data.get("results")) == expected_results - object_tags = response.data.get("results") - for object_tag in object_tags: + assert len(response.data) == expected_count + for object_tag in response.data: assert object_tag.get("is_valid") is True assert object_tag.get("taxonomy_id") == self.enabled_taxonomy.pk @@ -537,51 +532,6 @@ def test_retrieve_object_tags_invalid_taxonomy_queryparam(self, user_attr, objec response = self.client.get(url, {"taxonomy": 123123}) assert response.status_code == expected_status - @ddt.data( - # Page 1, default page size 10, total count 200, returns 10 results - (None, 1, None, status.HTTP_403_FORBIDDEN, None, None), - ("user", 1, None, status.HTTP_200_OK, 200, 10), - ("staff", 1, None, status.HTTP_200_OK, 200, 10), - # Page 2, default page size 10, total count 200, returns 10 results - (None, 2, None, status.HTTP_403_FORBIDDEN, None, None), - ("user", 2, None, status.HTTP_200_OK, 200, 10), - ("staff", 2, None, status.HTTP_200_OK, 200, 10), - # Page 21, default page size 10, total count 200, no more results - (None, 21, None, status.HTTP_403_FORBIDDEN, None, None), - ("user", 21, None, status.HTTP_404_NOT_FOUND, None, None), - ("staff", 21, None, status.HTTP_404_NOT_FOUND, None, None), - # Page 3, page size 2, total count 200, returns 2 results - (None, 3, 2, status.HTTP_403_FORBIDDEN, 200, 2), - ("user", 3, 2, status.HTTP_200_OK, 200, 2), - ("staff", 3, 2, status.HTTP_200_OK, 200, 2), - ) - @ddt.unpack - def test_retrieve_object_tags_pagination( - self, user_attr, page, page_size, expected_status, expected_count, expected_results - ): - """ - Test pagination for retrieve object tags - """ - url = OBJECT_TAGS_RETRIEVE_URL.format(object_id="abc") - - if user_attr: - user = getattr(self, user_attr) - self.client.force_authenticate(user=user) - - query_params = {"taxonomy": self.open_taxonomy_enabled.pk, "page": page} - if page_size: - query_params["page_size"] = page_size - - response = self.client.get(url, query_params) - assert response.status_code == expected_status - if status.is_success(expected_status): - assert response.data.get("count") == expected_count - assert response.data.get("results") is not None - assert len(response.data.get("results")) == expected_results - object_tags = response.data.get("results") - for object_tag in object_tags: - assert object_tag.get("taxonomy_id") == self.open_taxonomy_enabled.pk - @ddt.data( (None, "POST", status.HTTP_403_FORBIDDEN), (None, "PATCH", status.HTTP_403_FORBIDDEN), @@ -660,8 +610,8 @@ def test_tag_object(self, user_attr, taxonomy_attr, tag_values, expected_status) 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.get("results")) == len(tag_values) - assert set(t["value"] for t in response.data["results"]) == set(tag_values) + 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 to a closed taxonomy @@ -727,8 +677,8 @@ def test_tag_object_clear(self, user_attr, taxonomy_attr, tag_values, expected_s 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.get("results")) == len(tag_values) - assert set(t["value"] for t in response.data["results"]) == set(tag_values) + assert len(response.data) == len(tag_values) + assert set(t["value"] for t in response.data) == set(tag_values) @ddt.data( # Users and staff can add multiple tags to a allow_multiple=True taxonomy @@ -764,8 +714,8 @@ def test_tag_object_multiple(self, user_attr, taxonomy_attr, tag_values, expecte 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.get("results")) == len(tag_values) - assert set(t["value"] for t in response.data["results"]) == set(tag_values) + assert len(response.data) == len(tag_values) + assert set(t["value"] for t in response.data) == set(tag_values) @ddt.data( (None, status.HTTP_403_FORBIDDEN), @@ -782,3 +732,7 @@ def test_tag_object_without_permission(self, user_attr, expected_status): response = self.client.put(url, {"tags": ["Tag 1"]}, format="json") assert response.status_code == expected_status + if status.is_success(expected_status): + assert len(response.data) == 1 + assert set(t["value"] for t in response.data) == set(["Tag 1"]) + From def631f97bde8eb7b83ad207cd87c5d03dd03cba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 19 Sep 2023 12:16:00 -0300 Subject: [PATCH 02/13] test: clean unused code --- tests/openedx_tagging/core/tagging/test_views.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index e698a1216..21269f862 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -732,7 +732,4 @@ def test_tag_object_without_permission(self, user_attr, expected_status): response = self.client.put(url, {"tags": ["Tag 1"]}, format="json") assert response.status_code == expected_status - if status.is_success(expected_status): - assert len(response.data) == 1 - assert set(t["value"] for t in response.data) == set(["Tag 1"]) - + assert not status.is_success(expected_status) # No success cases here From 73aad4ece334cc98dad26631f509407d44c2776a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 19 Sep 2023 13:31:39 -0300 Subject: [PATCH 03/13] feat: add tag limit --- openedx_tagging/core/tagging/models/base.py | 13 +++++++ .../openedx_tagging/core/tagging/test_api.py | 35 +++++++++++++++++-- .../core/tagging/test_models.py | 11 ++++++ .../core/tagging/test_views.py | 26 +++++++++++++- 4 files changed, 81 insertions(+), 4 deletions(-) diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index 20f610e49..c7a6b4abb 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -404,6 +404,19 @@ def _find_object_tag_index(tag_ref, object_tags) -> int: -1, ) + def _check_current_tag_count() -> None: + """ + Checks if the current count of tags for the object is less than 100 + """ + current_count = ObjectTag.objects.filter(object_id=object_id).exclude(taxonomy_id=self.id).count() + + if current_count >= 100: + raise ValueError( + _(f"Object ({object_id}) already have 100 tags.") + ) + + _check_current_tag_count() + if not isinstance(tags, list): raise ValueError(_(f"Tags must be a list, not {type(tags).__name__}.")) diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 46e8c190e..42c44331f 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -54,7 +54,7 @@ def test_bad_taxonomy_class(self) -> None: def test_get_taxonomy(self) -> None: tax1 = tagging_api.get_taxonomy(1) assert tax1 == self.taxonomy - no_tax = tagging_api.get_taxonomy(10) + no_tax = tagging_api.get_taxonomy(200) assert no_tax is None def test_get_taxonomies(self) -> None: @@ -70,7 +70,7 @@ def test_get_taxonomies(self) -> None: 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" @@ -92,7 +92,7 @@ def test_get_taxonomies(self) -> None: self.taxonomy, self.system_taxonomy, self.user_taxonomy, - ] + ] + self.dummy_taxonomies @override_settings(LANGUAGES=test_languages) def test_get_tags(self) -> None: @@ -539,6 +539,35 @@ def test_tag_object_model_system_taxonomy_invalid(self) -> None: exc.exception ) + def test_tag_object_limit(self) -> None: + """ + Test that the tagging limit is enforced. + """ + # The user can add up to 100 tags to a object + for taxonomy in self.dummy_taxonomies: + tagging_api.tag_object( + taxonomy, + ["Dummy Tag"], + "object_1", + ) + + # Adding a new tag should fail + with self.assertRaises(ValueError) as exc: + tagging_api.tag_object( + self.taxonomy, + ["Eubacteria"], + "object_1", + ) + + # Updating existing tags should work + for taxonomy in self.dummy_taxonomies: + tagging_api.tag_object( + taxonomy, + ["New Dummy Tag"], + "object_1", + ) + + def test_get_object_tags(self) -> None: # Alpha tag has no taxonomy alpha = ObjectTag(object_id="abc") diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index 4b6f644aa..618db4272 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -93,6 +93,17 @@ def setUp(self): get_tag("System Tag 4"), ] + self.dummy_taxonomies = [] + for i in range(100): + taxonomy = Taxonomy.objects.create(name=f"ZZ Dummy Taxonomy {i:03}", allow_free_text=True) + ObjectTag.objects.create( + object_id="limit_tag_count", + taxonomy=taxonomy, + _name=taxonomy.name, + _value="Dummy Tag", + ) + self.dummy_taxonomies.append(taxonomy) + def setup_tag_depths(self): """ Annotate our tags with depth so we can compare them. diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 21269f862..379f58552 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -406,7 +406,7 @@ def _object_permission(_user, object_id: str) -> bool: """ Everyone have object permission on object_id "abc" """ - return object_id == "abc" + return object_id == "abc" or object_id == "limit_tag_count" super().setUp() @@ -458,6 +458,12 @@ def _object_permission(_user, object_id: str) -> bool: 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) + 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", _object_permission) @@ -733,3 +739,21 @@ def test_tag_object_without_permission(self, user_attr, expected_status): response = self.client.put(url, {"tags": ["Tag 1"]}, format="json") assert response.status_code == expected_status assert not status.is_success(expected_status) # No success cases here + + def test_tag_object_count_limit(self): + """ + Checks if the limit of 100 tags per object is enforced + """ + object_id = "limit_tag_count" + url = OBJECT_TAGS_UPDATE_URL.format(object_id=object_id, taxonomy_id=self.enabled_taxonomy.pk) + self.client.force_authenticate(user=self.staff) + response = self.client.put(url, {"tags": ["Tag 1"]}, format="json") + # Can't add another tag because the object already has 100 tags + assert response.status_code == status.HTTP_400_BAD_REQUEST + + + # The user can edit the tags that are already on the object + for taxonomy in self.dummy_taxonomies: + 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 From 876dfe57936e61be3b29ca21d4edc9e0a010e42b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 19 Sep 2023 15:03:50 -0300 Subject: [PATCH 04/13] refactor: add comment and update error message --- openedx_tagging/core/tagging/models/base.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index c7a6b4abb..90571a28e 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -408,11 +408,12 @@ def _check_current_tag_count() -> None: """ Checks if the current count of tags for the object is less than 100 """ + # Exclude self.id to avoid counting the tags that are going to be updated current_count = ObjectTag.objects.filter(object_id=object_id).exclude(taxonomy_id=self.id).count() if current_count >= 100: raise ValueError( - _(f"Object ({object_id}) already have 100 tags.") + _(f"Object ({object_id}) already have 100 or more tags.") ) _check_current_tag_count() From 40602f73f25dbb3b9b7f3883745c162fb450c762 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 19 Sep 2023 15:05:52 -0300 Subject: [PATCH 05/13] test: check error message --- tests/openedx_tagging/core/tagging/test_api.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 42c44331f..33acb4d59 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -558,6 +558,7 @@ def test_tag_object_limit(self) -> None: ["Eubacteria"], "object_1", ) + assert "already have 100 or more tags" in str(exc.exception) # Updating existing tags should work for taxonomy in self.dummy_taxonomies: From f014c9bd2da50b059158be3e66c932d6a48fd2e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 19 Sep 2023 15:23:40 -0300 Subject: [PATCH 06/13] style: fix pylint --- tests/openedx_tagging/core/tagging/test_views.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 379f58552..95a48fc57 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -406,7 +406,7 @@ def _object_permission(_user, object_id: str) -> bool: """ Everyone have object permission on object_id "abc" """ - return object_id == "abc" or object_id == "limit_tag_count" + return object_id in ("abc", "limit_tag_count") super().setUp() @@ -461,7 +461,12 @@ def _object_permission(_user, object_id: str) -> bool: self.dummy_taxonomies = [] for i in range(100): taxonomy = Taxonomy.objects.create(name=f"Dummy Taxonomy {i}", allow_free_text=True) - ObjectTag.objects.create(object_id="limit_tag_count", taxonomy=taxonomy, _name=taxonomy.name, _value="Dummy Tag") + 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 From c303cbb2c2f0a20878f67ba8498db1ab9403cc22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 19 Sep 2023 15:31:51 -0300 Subject: [PATCH 07/13] style: removing extra lines --- tests/openedx_tagging/core/tagging/test_api.py | 1 - tests/openedx_tagging/core/tagging/test_views.py | 1 - 2 files changed, 2 deletions(-) diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 33acb4d59..cd5d2baa3 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -568,7 +568,6 @@ def test_tag_object_limit(self) -> None: "object_1", ) - def test_get_object_tags(self) -> None: # Alpha tag has no taxonomy alpha = ObjectTag(object_id="abc") diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 95a48fc57..a1ec739d7 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -756,7 +756,6 @@ def test_tag_object_count_limit(self): # Can't add another tag because the object already has 100 tags assert response.status_code == status.HTTP_400_BAD_REQUEST - # The user can edit the tags that are already on the object for taxonomy in self.dummy_taxonomies: url = OBJECT_TAGS_UPDATE_URL.format(object_id=object_id, taxonomy_id=taxonomy.pk) From 432e4c22eb18a05eeafb5070a14666680f4c1484 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 21 Sep 2023 09:14:43 -0300 Subject: [PATCH 08/13] fix: validate allow_multiple tags --- openedx_tagging/core/tagging/models/base.py | 11 ++++++----- tests/openedx_tagging/core/tagging/test_api.py | 14 +++++++++++++- tests/openedx_tagging/core/tagging/test_models.py | 6 +++++- tests/openedx_tagging/core/tagging/test_views.py | 14 ++++++++++++-- 4 files changed, 36 insertions(+), 9 deletions(-) diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index 876ef439d..f102cfce2 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -446,25 +446,26 @@ def _find_object_tag_index(tag_ref, object_tags) -> int: -1, ) - def _check_current_tag_count() -> None: + def _check_new_tag_count(new_tag_count: int) -> None: """ - Checks if the current count of tags for the object is less than 100 + Checks if the new count of tags for the object is equal or less than 100 """ # Exclude self.id to avoid counting the tags that are going to be updated current_count = ObjectTag.objects.filter(object_id=object_id).exclude(taxonomy_id=self.id).count() - if current_count >= 100: + if current_count + new_tag_count > 100: raise ValueError( - _(f"Object ({object_id}) already have 100 or more tags.") + _(f"Cannot add more than 100 tags to ({object_id}).") ) - _check_current_tag_count() if not isinstance(tags, list): raise ValueError(_(f"Tags must be a list, not {type(tags).__name__}.")) tags = list(dict.fromkeys(tags)) # Remove duplicates preserving order + _check_new_tag_count(len(tags)) + if not self.allow_multiple and len(tags) > 1: raise ValueError(_(f"Taxonomy ({self.id}) only allows one tag per object.")) diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index af55af10c..3525bc5c9 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -606,7 +606,8 @@ def test_tag_object_limit(self) -> None: ["Eubacteria"], "object_1", ) - assert "already have 100 or more tags" in str(exc.exception) + assert exc.exception + assert "Cannot add more than 100 tags to" in str(exc.exception) # Updating existing tags should work for taxonomy in self.dummy_taxonomies: @@ -616,6 +617,17 @@ def test_tag_object_limit(self) -> None: "object_1", ) + # Updating existing tags adding a new one should fail + for taxonomy in self.dummy_taxonomies: + with self.assertRaises(ValueError) as exc: + tagging_api.tag_object( + taxonomy, + ["New Dummy Tag 1", "New Dummy Tag 2"], + "object_1", + ) + assert exc.exception + 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 = ObjectTag(object_id="abc") diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index f5ce65202..44905c3ad 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -117,7 +117,11 @@ def setUp(self): self.dummy_taxonomies = [] for i in range(100): - taxonomy = Taxonomy.objects.create(name=f"ZZ Dummy Taxonomy {i:03}", allow_free_text=True) + taxonomy = Taxonomy.objects.create( + name=f"ZZ Dummy Taxonomy {i:03}", + allow_free_text=True, + allow_multiple=True + ) ObjectTag.objects.create( object_id="limit_tag_count", taxonomy=taxonomy, diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 3366b5e92..b7b12cd31 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -469,7 +469,11 @@ def _object_permission(_user, object_id: str) -> bool: self.dummy_taxonomies = [] for i in range(100): - taxonomy = Taxonomy.objects.create(name=f"Dummy Taxonomy {i}", allow_free_text=True) + 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, @@ -771,6 +775,12 @@ def test_tag_object_count_limit(self): 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: + url = OBJECT_TAGS_UPDATE_URL.format(object_id=object_id, taxonomy_id=taxonomy.pk) + response = self.client.put(url, {"tags": ["New Tag 1", "New Tag 2"]}, format="json") + assert response.status_code == status.HTTP_400_BAD_REQUEST + class TestTaxonomyTagsView(TestTaxonomyViewMixin): """ @@ -1085,4 +1095,4 @@ def test_next_children(self): ) assert data.get("count") == self.children_tags_count[0] assert data.get("num_pages") == 2 - assert data.get("current_page") == 2 \ No newline at end of file + assert data.get("current_page") == 2 From 54d3fbd8df8c326f3266b5d48ae90ba65dfe0b34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 21 Sep 2023 09:24:03 -0300 Subject: [PATCH 09/13] chore: trigger CD/CI From c75751e2374fe1363731743b0b5ef7a0dbfaae9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 21 Sep 2023 09:29:31 -0300 Subject: [PATCH 10/13] chore: bump version --- 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 75d55cb5e..8b70cf96d 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -1,4 +1,4 @@ """ Open edX Learning ("Learning Core"). """ -__version__ = "0.1.6" +__version__ = "0.1.7" From 005379d0c614fda97e7ea341b385299f9c77f8ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 21 Sep 2023 09:47:19 -0300 Subject: [PATCH 11/13] style: fix pylint --- openedx_tagging/core/tagging/models/base.py | 1 - openedx_tagging/core/tagging/rest_api/v1/views.py | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index f102cfce2..abb4eae1d 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -458,7 +458,6 @@ def _check_new_tag_count(new_tag_count: int) -> None: _(f"Cannot add more than 100 tags to ({object_id}).") ) - if not isinstance(tags, list): raise ValueError(_(f"Tags must be a list, not {type(tags).__name__}.")) diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index 9ab14aa8a..d23be71a8 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -499,4 +499,5 @@ def get_queryset(self) -> list[Tag]: # type: ignore[override] # This function is not called automatically self.pagination_class = self.get_pagination_class() - return result \ No newline at end of file + return result + From 862ee9175056a5401781486858424a3619661afd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 21 Sep 2023 09:55:07 -0300 Subject: [PATCH 12/13] style: pylint --- openedx_tagging/core/tagging/rest_api/v1/views.py | 1 - 1 file changed, 1 deletion(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index d23be71a8..d869c765d 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -500,4 +500,3 @@ def get_queryset(self) -> list[Tag]: # type: ignore[override] self.pagination_class = self.get_pagination_class() return result - From 8e875b6d54411f9629380aa7d3eb91209215bf66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 21 Sep 2023 09:59:57 -0300 Subject: [PATCH 13/13] style: isort --- openedx_tagging/core/tagging/rest_api/v1/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index d869c765d..91208a0f3 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -7,8 +7,8 @@ from django.http import Http404 from rest_framework import mixins from rest_framework.exceptions import MethodNotAllowed, PermissionDenied, ValidationError -from rest_framework.response import Response from rest_framework.generics import ListAPIView +from rest_framework.response import Response from rest_framework.viewsets import GenericViewSet, ModelViewSet from openedx_tagging.core.tagging.models.base import Tag