From f763c0e1cf72e7a70f87b0fdf91cdbcd4cb90407 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Wed, 11 Oct 2023 17:46:07 +0300 Subject: [PATCH 01/13] feat: Implement add taxonomy tag api/rest + tests --- openedx_tagging/core/tagging/api.py | 14 ++ openedx_tagging/core/tagging/models/base.py | 39 +++++ .../core/tagging/rest_api/v1/serializers.py | 13 ++ .../core/tagging/rest_api/v1/views.py | 62 +++++++- .../core/tagging/test_views.py | 142 +++++++++++++++++- 5 files changed, 264 insertions(+), 6 deletions(-) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index e9140968..2c78b10c 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -315,3 +315,17 @@ def autocomplete_tags( # remove repeats .distinct() ) + + +def add_tag_to_taxonomy( + taxonomy: Taxonomy, + tag: str, + parent_tag_id: int | None = None, + external_id: str | None = None +) -> Tag: + """ + Adds a new Tag to provided Taxonomy. If a Tag already exists in the + Taxonomy, an exception is raised, otherwise the newly created + Tag is returned + """ + return taxonomy.cast().add_tag(tag, parent_tag_id, external_id) diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index d24d67d4..ffcef13e 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -342,6 +342,45 @@ def get_filtered_tags( return tag_set.order_by("value", "id") + def add_tag( + self, + tag_value: str, + parent_tag_id: int | None = None, + external_id: str | None = None + + ) -> Tag: + """ + Add new Tag to Taxonomy. If an existing Tag with the `tag_value` already + exists in the Taxonomy, an exception is raised, otherwise the newly + created Tag is returned + """ + self.check_casted() + + if self.allow_free_text: + raise ValueError( + "add_tag() doesn't work for free text taxonomies. They don't use Tag instances." + ) + + if self.system_defined: + raise ValueError( + "add_tag() doesn't work for system defined taxonomies. They cannot be modified." + ) + + current_tags = self.get_tags() + + if self.tag_set.filter(value__iexact=tag_value).exists(): + raise ValueError(f"Tag with value '{tag_value}' already exists for taxonomy.") + + parent = None + if parent_tag_id: + parent = Tag.objects.get(id=parent_tag_id) + + tag = Tag.objects.create( + taxonomy=self, value=tag_value, parent=parent, external_id=external_id + ) + + return tag + def validate_value(self, value: str) -> bool: """ Check if 'value' is part of this Taxonomy. diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index a4eb89ff..b6912839 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -110,6 +110,7 @@ class Meta: "value", "taxonomy_id", "parent_id", + "external_id", "sub_tags_link", "children_count", ) @@ -192,3 +193,15 @@ def get_children_count(self, obj): Returns the number of child tags of the given tag. """ return len(obj.sub_tags) + + +class TaxonomyTagCreateBodySerializer(serializers.Serializer): # pylint: disable=abstract-method + """ + Serializer of the body for the Taxonomy Tags CREATE view + """ + + tag = serializers.CharField(required=True) + parent_tag_id = serializers.PrimaryKeyRelatedField( + queryset=Tag.objects.all(), required=False + ) + external_id = serializers.CharField(required=False) diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index 743f5bcd..4b42fc60 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -3,12 +3,12 @@ """ from __future__ import annotations -from django.db import models +from django.db import model from django.http import Http404, HttpResponse -from rest_framework import mixins +from rest_framework import mixins, status from rest_framework.decorators import action from rest_framework.exceptions import MethodNotAllowed, PermissionDenied, ValidationError -from rest_framework.generics import ListAPIView +from rest_framework.generics import ListAPIView, RetrieveUpdateDestroyAPIView from rest_framework.response import Response from rest_framework.viewsets import GenericViewSet, ModelViewSet @@ -23,6 +23,7 @@ get_taxonomy, search_tags, tag_object, + add_tag_to_taxonomy, ) from ...import_export.api import export_tags from ...import_export.parsers import ParserFormat @@ -41,6 +42,7 @@ TaxonomyExportQueryParamsSerializer, TaxonomyListQueryParamsSerializer, TaxonomySerializer, + TaxonomyTagCreateBodySerializer, ) from .utils import view_auth_classes @@ -404,9 +406,9 @@ def update(self, request, *args, **kwargs) -> Response: @view_auth_classes -class TaxonomyTagsView(ListAPIView): +class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): """ - View to list tags of a taxonomy. + View to list/create/update/delete tags of a taxonomy. **List Query Parameters** * pk (required) - The pk of the taxonomy to retrieve tags. @@ -423,6 +425,31 @@ class TaxonomyTagsView(ListAPIView): * 400 - Invalid query parameter * 403 - Permission denied * 404 - Taxonomy not found + + **Create Query Parameters** + * pk (required) - The pk of the taxonomy to create a Tag for + + **Create Request Body** + * tag (required): The value of the Tag that should be added to + the Taxonomy + * parent_tag_id (optional): The id of the parent tag that the new + Tag should fall under + * extenal_id (optional): The external id for the new Tag + + **Create Example Requests** + POST api/tagging/v1/taxonomy/:pk/tags - Create a Tag in taxonomy + { + "value": "New Tag", + "parent_tag_id": 123 + "external_id": "abc123", + } + + **Create Query Returns** + * 201 - Success + * 400 - Invalid parameters provided + * 403 - Permission denied + * 404 - Taxonomy not found + """ permission_classes = [TagListPermissions] @@ -580,3 +607,28 @@ def get_queryset(self) -> list[Tag]: # type: ignore[override] self.pagination_class = self.get_pagination_class() return result + + def post(self, request, *args, **kwargs): + """ + Creates new Tag in Taxonomy and returns the newly created Tag. + """ + pk = self.kwargs.get("pk") + taxonomy = self.get_taxonomy(pk) + + body = TaxonomyTagCreateBodySerializer(data=request.data) + body.is_valid(raise_exception=True) + + tag = body.data.get("tag") + parent_tag_id = body.data.get("parent_tag_id", None) + external_id = body.data.get("external_id", None) + + try: + new_tag = add_tag_to_taxonomy( + taxonomy, tag, parent_tag_id, external_id + ) + except ValueError as e: + raise ValidationError from e + + return Response( + TagsSerializer(new_tag).data, status=status.HTTP_201_CREATED + ) diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index d97296ba..edfe4a19 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -897,7 +897,7 @@ def test_tag_object_count_limit(self): class TestTaxonomyTagsView(TestTaxonomyViewMixin): """ - Tests the list tags of taxonomy view + Tests the list/create/update/delete tags of taxonomy view """ fixtures = ["tests/openedx_tagging/core/fixtures/tagging.yaml"] @@ -1209,3 +1209,143 @@ 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 + + def test_create_tag_in_taxonomy(self): + self.client.force_authenticate(user=self.user) + new_tag_value = "New Tag" + + create_data = { + "tag": new_tag_value + } + + response = self.client.post( + self.small_taxonomy_url, create_data, format="json" + ) + + assert response.status_code == status.HTTP_201_CREATED + + data = response.data + + self.assertIsNotNone(data.get("id")) + self.assertEqual(data.get("value"), new_tag_value) + self.assertEqual(data.get("taxonomy_id"), self.small_taxonomy.pk) + self.assertIsNone(data.get("parent_id")) + self.assertIsNone(data.get("external_id")) + self.assertIsNone(data.get("sub_tags_link")) + self.assertEqual(data.get("children_count"), 0) + + def test_create_tag_in_taxonomy_with_parent_id(self): + self.client.force_authenticate(user=self.user) + parent_tag = self.small_taxonomy.tag_set.filter(parent=None).first() + new_tag_value = "New Child Tag" + new_external_id = "extId" + + create_data = { + "tag": new_tag_value, + "parent_tag_id": parent_tag.id, + "external_id": new_external_id + } + + response = self.client.post( + self.small_taxonomy_url, create_data, format="json" + ) + + assert response.status_code == status.HTTP_201_CREATED + + data = response.data + + self.assertIsNotNone(data.get("id")) + self.assertEqual(data.get("value"), new_tag_value) + self.assertEqual(data.get("taxonomy_id"), self.small_taxonomy.pk) + self.assertEqual(data.get("parent_id"), parent_tag.id) + self.assertEqual(data.get("external_id"), new_external_id) + self.assertIsNone(data.get("sub_tags_link")) + self.assertEqual(data.get("children_count"), 0) + + def test_create_tag_in_invalid_taxonomy(self): + self.client.force_authenticate(user=self.user) + new_tag_value = "New Tag" + + create_data = { + "tag": new_tag_value + } + + invalid_taxonomy_url = TAXONOMY_TAGS_URL.format(pk=919191) + response = self.client.post( + invalid_taxonomy_url, create_data, format="json" + ) + + assert response.status_code == status.HTTP_404_NOT_FOUND + + def test_create_tag_in_free_text_taxonomy(self): + self.client.force_authenticate(user=self.user) + new_tag_value = "New Tag" + + create_data = { + "tag": new_tag_value + } + + # Setting free text flag on taxonomy + self.small_taxonomy.allow_free_text = True + self.small_taxonomy.save() + + response = self.client.post( + self.small_taxonomy_url, create_data, format="json" + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + + def test_create_tag_in_system_defined_taxonomy(self): + self.client.force_authenticate(user=self.user) + new_tag_value = "New Tag" + + create_data = { + "tag": new_tag_value + } + + # Setting taxonomy to be system defined + self.small_taxonomy.taxonomy_class = SystemDefinedTaxonomy + self.small_taxonomy.save() + + response = self.client.post( + self.small_taxonomy_url, create_data, format="json" + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + + def test_create_tag_in_taxonomy_with_invalid_parent_tag_id(self): + self.client.force_authenticate(user=self.user) + invalid_parent_tag_id = 91919 + new_tag_value = "New Child Tag" + + create_data = { + "tag": new_tag_value, + "parent_tag_id": invalid_parent_tag_id, + } + + response = self.client.post( + self.small_taxonomy_url, create_data, format="json" + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + + def test_create_tag_in_taxonomy_with_already_existing_value(self): + self.client.force_authenticate(user=self.user) + new_tag_value = "New Tag" + + create_data = { + "tag": new_tag_value + } + + response = self.client.post( + self.small_taxonomy_url, create_data, format="json" + ) + + assert response.status_code == status.HTTP_201_CREATED + + # Make request again with the same Tag value after it was created + response = self.client.post( + self.small_taxonomy_url, create_data, format="json" + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST From 6f5c5a10656eb4d7baadfc3ada31362d1e149dca Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Thu, 12 Oct 2023 17:14:57 +0300 Subject: [PATCH 02/13] feat: Add update taxonomy tag api/rest + tests Also fixed a few things in the add taxonomy tag rest api --- openedx_tagging/core/tagging/api.py | 18 +- openedx_tagging/core/tagging/models/base.py | 46 ++++- .../core/tagging/rest_api/v1/serializers.py | 11 ++ .../core/tagging/rest_api/v1/views.py | 39 +++- .../core/tagging/test_views.py | 178 ++++++++++++++++++ 5 files changed, 279 insertions(+), 13 deletions(-) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index 2c78b10c..a8a4e7bc 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -129,13 +129,7 @@ def resync_object_tags(object_tags: QuerySet | None = None) -> int: if not object_tags: object_tags = ObjectTag.objects.select_related("tag", "taxonomy") - num_changed = 0 - for object_tag in object_tags: - changed = object_tag.resync() - if changed: - object_tag.save() - num_changed += 1 - return num_changed + return ObjectTag.resync_object_tags(object_tags) def get_object_tags( @@ -329,3 +323,13 @@ def add_tag_to_taxonomy( Tag is returned """ return taxonomy.cast().add_tag(tag, parent_tag_id, external_id) + + +def update_tag_in_taxonomy(taxonomy: Taxonomy, tag: int, tag_value: str): + """ + Update a Tag that belongs to a Taxonomy. The related ObjectTags are + updated accordingly. + + Currently only support updates the Tag value. + """ + return taxonomy.cast().update_tag(tag, tag_value) diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index ffcef13e..5e973c51 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -366,14 +366,12 @@ def add_tag( "add_tag() doesn't work for system defined taxonomies. They cannot be modified." ) - current_tags = self.get_tags() - if self.tag_set.filter(value__iexact=tag_value).exists(): raise ValueError(f"Tag with value '{tag_value}' already exists for taxonomy.") parent = None if parent_tag_id: - parent = Tag.objects.get(id=parent_tag_id) + parent = self.tag_set.get(id=parent_tag_id) tag = Tag.objects.create( taxonomy=self, value=tag_value, parent=parent, external_id=external_id @@ -381,6 +379,33 @@ def add_tag( return tag + def update_tag(self, tag_id: int, tag_value: str) -> Tag: + """ + Update an existing Tag in Taxonomy and return it. Currently only + supports updating the Tag's value. + """ + self.check_casted() + + if self.allow_free_text: + raise ValueError( + "update_tag() doesn't work for free text taxonomies. They don't use Tag instances." + ) + + if self.system_defined: + raise ValueError( + "update_tag() doesn't work for system defined taxonomies. They cannot be modified." + ) + + # Update Tag instance with new value + tag = self.tag_set.get(id=tag_id) + tag.value = tag_value + tag.save() + + # Resync all related ObjectTags to update to the new Tag value + object_tags = self.objecttag_set.all() + ObjectTag.resync_object_tags(object_tags) + return tag + def validate_value(self, value: str) -> bool: """ Check if 'value' is part of this Taxonomy. @@ -648,3 +673,18 @@ def copy(self, object_tag: ObjectTag) -> Self: self._value = object_tag._value # pylint: disable=protected-access self._name = object_tag._name # pylint: disable=protected-access return self + + @classmethod + def resync_object_tags(cls, object_tags: models.QuerySet[ObjectTag]) -> int: + """ + Reconciles ObjectTag entries with any changes made to their associated + taxonomies and tags. Return the number of changes made. + """ + num_changed = 0 + for object_tag in object_tags: + changed = object_tag.resync() + if changed: + object_tag.save() + num_changed += 1 + + return num_changed diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index b6912839..fb6d6c84 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -205,3 +205,14 @@ class TaxonomyTagCreateBodySerializer(serializers.Serializer): # pylint: disabl queryset=Tag.objects.all(), required=False ) external_id = serializers.CharField(required=False) + + +class TaxonomyTagUpdateBodySerializer(serializers.Serializer): # pylint: disable=abstract-method + """ + Serializer of the body for the Taxonomy Tags UPDATE view + """ + + tag = serializers.PrimaryKeyRelatedField( + queryset=Tag.objects.all(), required=True + ) + tag_value = serializers.CharField(required=True) diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index 4b42fc60..fa572f42 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -3,7 +3,7 @@ """ from __future__ import annotations -from django.db import model +from django.db import models from django.http import Http404, HttpResponse from rest_framework import mixins, status from rest_framework.decorators import action @@ -15,6 +15,7 @@ from openedx_tagging.core.tagging.models.base import Tag from ...api import ( + add_tag_to_taxonomy, create_taxonomy, get_children_tags, get_object_tags, @@ -23,7 +24,7 @@ get_taxonomy, search_tags, tag_object, - add_tag_to_taxonomy, + update_tag_in_taxonomy, ) from ...import_export.api import export_tags from ...import_export.parsers import ParserFormat @@ -43,6 +44,7 @@ TaxonomyListQueryParamsSerializer, TaxonomySerializer, TaxonomyTagCreateBodySerializer, + TaxonomyTagUpdateBodySerializer, ) from .utils import view_auth_classes @@ -626,9 +628,40 @@ def post(self, request, *args, **kwargs): new_tag = add_tag_to_taxonomy( taxonomy, tag, parent_tag_id, external_id ) + except Tag.DoesNotExist as e: + raise Http404("Parent Tag not found") from e + except ValueError as e: + raise ValidationError from e + + serializer_context = self.get_serializer_context() + return Response( + self.serializer_class(new_tag, context=serializer_context).data, + status=status.HTTP_201_CREATED + ) + + def update(self, request, *args, **kwargs): + """ + Updates a Tag that belongs to the Taxonomy and returns it. + Currently only updating the Tag value is supported. + """ + pk = self.kwargs.get("pk") + taxonomy = self.get_taxonomy(pk) + + body = TaxonomyTagUpdateBodySerializer(data=request.data) + body.is_valid(raise_exception=True) + + tag = body.data.get("tag") + tag_value = body.data.get("tag_value") + + try: + updated_tag = update_tag_in_taxonomy(taxonomy, tag, tag_value) + except Tag.DoesNotExist as e: + raise Http404("Tag not found") from e except ValueError as e: raise ValidationError from e + serializer_context = self.get_serializer_context() return Response( - TagsSerializer(new_tag).data, status=status.HTTP_201_CREATED + self.serializer_class(updated_tag, context=serializer_context).data, + status=status.HTTP_200_OK ) diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index edfe4a19..a007b2bb 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -1329,6 +1329,22 @@ def test_create_tag_in_taxonomy_with_invalid_parent_tag_id(self): assert response.status_code == status.HTTP_400_BAD_REQUEST + def test_create_tag_in_taxonomy_with_parent_tag_id_in_other_taxonomy(self): + self.client.force_authenticate(user=self.user) + invalid_parent_tag_id = 1 + new_tag_value = "New Child Tag" + + create_data = { + "tag": new_tag_value, + "parent_tag_id": invalid_parent_tag_id, + } + + response = self.client.post( + self.large_taxonomy_url, create_data, format="json" + ) + + assert response.status_code == status.HTTP_404_NOT_FOUND + def test_create_tag_in_taxonomy_with_already_existing_value(self): self.client.force_authenticate(user=self.user) new_tag_value = "New Tag" @@ -1349,3 +1365,165 @@ def test_create_tag_in_taxonomy_with_already_existing_value(self): ) assert response.status_code == status.HTTP_400_BAD_REQUEST + + def test_update_tag_in_taxonomy_with_different_methods(self): + self.client.force_authenticate(user=self.user) + updated_tag_value = "Updated Tag" + updated_tag_value_2 = "Updated Tag 2" + + # Existing Tag that will be updated + existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() + + update_data = { + "tag": existing_tag.id, + "tag_value": updated_tag_value + } + + # Test updating using the PUT method + response = self.client.put( + self.small_taxonomy_url, update_data, format="json" + ) + + assert response.status_code == status.HTTP_200_OK + + data = response.data + + # Check that Tag value got updated + self.assertEqual(data.get("id"), existing_tag.id) + self.assertEqual(data.get("value"), updated_tag_value) + self.assertEqual(data.get("taxonomy_id"), self.small_taxonomy.pk) + self.assertEqual(data.get("parent_id"), existing_tag.parent) + self.assertEqual(data.get("external_id"), existing_tag.external_id) + + # Test updating using the PATCH method + update_data["tag_value"] = updated_tag_value_2 + response = self.client.patch( + self.small_taxonomy_url, update_data, format="json" + ) + + assert response.status_code == status.HTTP_200_OK + + data = response.data + + # Check the Tag value got updated again + self.assertEqual(data.get("id"), existing_tag.id) + self.assertEqual(data.get("value"), updated_tag_value_2) + self.assertEqual(data.get("taxonomy_id"), self.small_taxonomy.pk) + self.assertEqual(data.get("parent_id"), existing_tag.parent) + self.assertEqual(data.get("external_id"), existing_tag.external_id) + + def test_update_tag_in_taxonomy_reflects_changes_in_object_tags(self): + self.client.force_authenticate(user=self.user) + + existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() + + # Setup ObjectTags + # _value=existing_tag.value + object_tag_1 = ObjectTag.objects.create( + object_id="abc", taxonomy=self.small_taxonomy, tag=existing_tag + ) + object_tag_2 = ObjectTag.objects.create( + object_id="def", taxonomy=self.small_taxonomy, tag=existing_tag + ) + object_tag_3 = ObjectTag.objects.create( + object_id="ghi", taxonomy=self.small_taxonomy, tag=existing_tag + ) + + assert object_tag_1.value == existing_tag.value + assert object_tag_2.value == existing_tag.value + assert object_tag_3.value == existing_tag.value + + updated_tag_value = "Updated Tag" + update_data = { + "tag": existing_tag.id, + "tag_value": updated_tag_value + } + + # Test updating using the PUT method + response = self.client.put( + self.small_taxonomy_url, update_data, format="json" + ) + + assert response.status_code == status.HTTP_200_OK + + data = response.data + + # Check that Tag value got updated + self.assertEqual(data.get("id"), existing_tag.id) + self.assertEqual(data.get("value"), updated_tag_value) + self.assertEqual(data.get("taxonomy_id"), self.small_taxonomy.pk) + self.assertEqual(data.get("parent_id"), existing_tag.parent) + self.assertEqual(data.get("external_id"), existing_tag.external_id) + + # Check that the ObjectTags got updated as well + object_tag_1.refresh_from_db() + self.assertEqual(object_tag_1.value, updated_tag_value) + object_tag_2.refresh_from_db() + self.assertEqual(object_tag_2.value, updated_tag_value) + object_tag_3.refresh_from_db() + self.assertEqual(object_tag_3.value, updated_tag_value) + + def test_update_tag_in_taxonomy_with_invalid_tag_id(self): + self.client.force_authenticate(user=self.user) + updated_tag_value = "Updated Tag" + + update_data = { + "tag": 919191, + "tag_value": updated_tag_value + } + + response = self.client.put( + self.small_taxonomy_url, update_data, format="json" + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + + def test_update_tag_in_taxonomy_with_tag_id_in_other_taxonomy(self): + self.client.force_authenticate(user=self.user) + updated_tag_value = "Updated Tag" + + update_data = { + "tag": 1, + "tag_value": updated_tag_value + } + + response = self.client.put( + self.large_taxonomy_url, update_data, format="json" + ) + + assert response.status_code == status.HTTP_404_NOT_FOUND + + def test_update_tag_in_taxonomy_with_no_tag_value_provided(self): + self.client.force_authenticate(user=self.user) + + # Existing Tag that will be updated + existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() + + update_data = { + "tag": existing_tag.id + } + + response = self.client.put( + self.small_taxonomy_url, update_data, format="json" + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + + def test_update_tag_in_invalid_taxonomy(self): + self.client.force_authenticate(user=self.user) + + # Existing Tag that will be updated + existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() + + updated_tag_value = "Updated Tag" + update_data = { + "tag": existing_tag.id, + "tag_value": updated_tag_value + } + + invalid_taxonomy_url = TAXONOMY_TAGS_URL.format(pk=919191) + response = self.client.put( + invalid_taxonomy_url, update_data, format="json" + ) + + assert response.status_code == status.HTTP_404_NOT_FOUND From 079f1d740a0ffb0f33104154b957c44c115c11f2 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Sun, 15 Oct 2023 15:06:36 +0300 Subject: [PATCH 03/13] feat: Add delete taxonomy tags api/rest + tests --- openedx_tagging/core/tagging/api.py | 13 ++ openedx_tagging/core/tagging/models/base.py | 37 ++++++ .../core/tagging/rest_api/v1/serializers.py | 13 ++ .../core/tagging/rest_api/v1/views.py | 75 ++++++++++- .../core/tagging/test_views.py | 125 ++++++++++++++++++ 5 files changed, 261 insertions(+), 2 deletions(-) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index a8a4e7bc..19d4f11e 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -333,3 +333,16 @@ def update_tag_in_taxonomy(taxonomy: Taxonomy, tag: int, tag_value: str): Currently only support updates the Tag value. """ return taxonomy.cast().update_tag(tag, tag_value) + + +def delete_tags_from_taxonomy( + taxonomy: Taxonomy, + tag_ids: list[Tag], + with_subtags: bool +): + """ + Delete Tags that belong to a Taxonomy. If any of the Tags have children and + the `with_subtags` is not set to `True` it will fail, otherwise + the sub-tags will be deleted as well. + """ + return taxonomy.cast().delete_tags(tag_ids, with_subtags) diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index 5e973c51..5fcdcb33 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -406,6 +406,43 @@ def update_tag(self, tag_id: int, tag_value: str) -> Tag: ObjectTag.resync_object_tags(object_tags) return tag + def delete_tags(self, tag_ids: List[int], with_subtags: bool = False): + """ + Delete the Taxonomy Tags provided. If any of them have children and + the `with_subtags` is not set to `True` it will fail, otherwise + the sub-tags will be deleted as well. + """ + self.check_casted() + + if self.allow_free_text: + raise ValueError( + "delete_tags() doesn't work for free text taxonomies. They don't use Tag instances." + ) + + if self.system_defined: + raise ValueError( + "delete_tags() doesn't work for system defined taxonomies. They cannot be modified." + ) + + tags = self.tag_set.filter(id__in=tag_ids) + + if tags.count() != len(tag_ids): + # If they do not match that means there is a Tag ID in the provided + # list that is either invalid or does not belong to this Taxonomy + raise ValueError("Invalid tag id provided or tag id does not belong to taxonomy") + + # Check if any Tag contains subtags (children) + contains_children = tags.filter(children__isnull=False).distinct().exists() + + if contains_children and not with_subtags: + raise ValueError( + "Tag(s) contain children, `with_subtags` must be `True` for " + "all Tags and their subtags (children) to be deleted." + ) + + # Delete the Tags with their subtags if any + tags.delete() + def validate_value(self, value: str) -> bool: """ Check if 'value' is part of this Taxonomy. diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index fb6d6c84..0a1ef3cb 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -216,3 +216,16 @@ class TaxonomyTagUpdateBodySerializer(serializers.Serializer): # pylint: disabl queryset=Tag.objects.all(), required=True ) tag_value = serializers.CharField(required=True) + + +class TaxonomyTagDeleteBodySerializer(serializers.Serializer): # pylint: disable=abstract-method + """ + Serializer of the body for the Taxonomy Tags DELETE view + """ + + tag_ids = serializers.PrimaryKeyRelatedField( + queryset=Tag.objects.all(), + many=True, + required=True + ) + with_subtags = serializers.BooleanField(required=False) diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index fa572f42..cf706010 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -17,6 +17,7 @@ from ...api import ( add_tag_to_taxonomy, create_taxonomy, + delete_tags_from_taxonomy, get_children_tags, get_object_tags, get_root_tags, @@ -44,6 +45,7 @@ TaxonomyListQueryParamsSerializer, TaxonomySerializer, TaxonomyTagCreateBodySerializer, + TaxonomyTagDeleteBodySerializer, TaxonomyTagUpdateBodySerializer, ) from .utils import view_auth_classes @@ -452,6 +454,53 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): * 403 - Permission denied * 404 - Taxonomy not found + **Update Query Parameters** + * pk (required) - The pk of the taxonomy to update a Tag in + + **Update Request Body** + * tag (required): The ID of the Tag that should be updated + * tag_value (required): The updated value of the Tag + + **Update Example Requests** + PUT api/tagging/v1/taxonomy/:pk/tags - Update a Tag in Taxonomy + { + "tag": 1, + "tag_value": "Updated Tag Value" + } + PATCH api/tagging/v1/taxonomy/:pk/tags - Update a Tag in Taxonomy + { + "tag": 1, + "tag_value": "Updated Tag Value" + } + + **Update Query Returns** + * 200 - Success + * 400 - Invalid parameters provided + * 403 - Permission denied + * 404 - Taxonomy, Tag or Parent Tag not found + + **Delete Query Parameters** + * pk (required) - The pk of the taxonomy to Delete Tag(s) in + + **Delete Request Body** + * tag_ids (required): The IDs of Tags that should be deleted from Taxonomy + * with_subtags (optional): If a Tag in the provided ids contains + children (subtags), deletion will fail unless + set to `True`. Defaults to `False`. + + **Delete Example Requests** + DELETE api/tagging/v1/taxonomy/:pk/tags - Delete Tag(s) in Taxonomy + { + "tag_ids": [1,2,3], + "with_subtags": True + } + + **Delete Query Returns** + * 200 - Success + * 400 - Invalid parameters provided + * 403 - Permission denied + * 404 - Taxonomy not found + """ permission_classes = [TagListPermissions] @@ -631,7 +680,7 @@ def post(self, request, *args, **kwargs): except Tag.DoesNotExist as e: raise Http404("Parent Tag not found") from e except ValueError as e: - raise ValidationError from e + raise ValidationError(e) from e serializer_context = self.get_serializer_context() return Response( @@ -658,10 +707,32 @@ def update(self, request, *args, **kwargs): except Tag.DoesNotExist as e: raise Http404("Tag not found") from e except ValueError as e: - raise ValidationError from e + raise ValidationError(e) from e serializer_context = self.get_serializer_context() return Response( self.serializer_class(updated_tag, context=serializer_context).data, status=status.HTTP_200_OK ) + + def delete(self, request, *args, **kwargs): + """ + Deletes Tag(s) in Taxonomy. If any of the Tags have children and + the `with_subtags` is not set to `True` it will fail, otherwise + the sub-tags will be deleted as well. + """ + pk = self.kwargs.get("pk") + taxonomy = self.get_taxonomy(pk) + + body = TaxonomyTagDeleteBodySerializer(data=request.data) + body.is_valid(raise_exception=True) + + tag_ids = body.data.get("tag_ids") + with_subtags = body.data.get("with_subtags") + + try: + delete_tags_from_taxonomy(taxonomy, tag_ids, with_subtags) + except ValueError as e: + raise ValidationError(e) from e + + return Response(status=status.HTTP_200_OK) diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index a007b2bb..a32bda4b 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -1527,3 +1527,128 @@ def test_update_tag_in_invalid_taxonomy(self): ) assert response.status_code == status.HTTP_404_NOT_FOUND + + def test_delete_single_tag_from_taxonomy(self): + self.client.force_authenticate(user=self.user) + + # Get Tag that will be deleted + existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() + + delete_data = { + "tag_ids": [existing_tag.id], + "with_subtags": True + } + + response = self.client.delete( + self.small_taxonomy_url, delete_data, format="json" + ) + + assert response.status_code == status.HTTP_200_OK + + # Check that Tag no longer exists + with self.assertRaises(Tag.DoesNotExist): + existing_tag.refresh_from_db() + + def test_delete_multiple_tags_from_taxonomy(self): + self.client.force_authenticate(user=self.user) + + # Get Tags that will be deleted + existing_tags = self.small_taxonomy.tag_set.filter(parent=None)[:3] + + delete_data = { + "tag_ids": [existing_tag.id for existing_tag in existing_tags], + "with_subtags": True + } + + response = self.client.delete( + self.small_taxonomy_url, delete_data, format="json" + ) + + assert response.status_code == status.HTTP_200_OK + + # Check that Tags no longer exists + for existing_tag in existing_tags: + with self.assertRaises(Tag.DoesNotExist): + existing_tag.refresh_from_db() + + def test_delete_tag_with_subtags_should_fail_without_flag_passed(self): + self.client.force_authenticate(user=self.user) + + # Get Tag that will be deleted + existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() + + delete_data = { + "tag_ids": [existing_tag.id] + } + + response = self.client.delete( + self.small_taxonomy_url, delete_data, format="json" + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + + def test_delete_tag_in_invalid_taxonomy(self): + self.client.force_authenticate(user=self.user) + + # Get Tag that will be deleted + existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() + + delete_data = { + "tag_ids": [existing_tag.id] + } + + invalid_taxonomy_url = TAXONOMY_TAGS_URL.format(pk=919191) + response = self.client.delete( + invalid_taxonomy_url, delete_data, format="json" + ) + + assert response.status_code == status.HTTP_404_NOT_FOUND + + def test_delete_tag_in_taxonomy_with_invalid_tag_id(self): + self.client.force_authenticate(user=self.user) + + delete_data = { + "tag_ids": [91919] + } + + response = self.client.delete( + self.small_taxonomy_url, delete_data, format="json" + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + + def test_delete_tag_with_tag_id_in_other_taxonomy(self): + self.client.force_authenticate(user=self.user) + + # Get Tag in other Taxonomy + tag_in_other_taxonomy = self.small_taxonomy.tag_set.filter(parent=None).first() + + delete_data = { + "tag_ids": [tag_in_other_taxonomy.id] + } + + response = self.client.delete( + self.large_taxonomy_url, delete_data, format="json" + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + + def test_delete_tag_in_taxonomy_without_subtags(self): + self.client.force_authenticate(user=self.user) + + # Get Tag that will be deleted + existing_tag = self.small_taxonomy.tag_set.filter(children__isnull=True).first() + + delete_data = { + "tag_ids": [existing_tag.id] + } + + response = self.client.delete( + self.small_taxonomy_url, delete_data, format="json" + ) + + assert response.status_code == status.HTTP_200_OK + + # Check that Tag no longer exists + with self.assertRaises(Tag.DoesNotExist): + existing_tag.refresh_from_db() From 7750ccd6c47fc0aabdb71c781541b525f07a3d33 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Sun, 15 Oct 2023 16:32:16 +0300 Subject: [PATCH 04/13] test: Add test cases for logged out user --- .../core/tagging/rest_api/v1/views.py | 7 +-- .../core/tagging/test_views.py | 46 +++++++++++++++++++ 2 files changed, 50 insertions(+), 3 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index cf706010..dc0c511e 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -15,6 +15,7 @@ from openedx_tagging.core.tagging.models.base import Tag from ...api import ( + TagDoesNotExist, add_tag_to_taxonomy, create_taxonomy, delete_tags_from_taxonomy, @@ -401,7 +402,7 @@ def update(self, request, *args, **kwargs) -> Response: tags = body.data.get("tags", []) try: tag_object(taxonomy, tags, object_id) - except Tag.DoesNotExist as e: + except TagDoesNotExist as e: raise ValidationError from e except ValueError as e: raise ValidationError from e @@ -677,7 +678,7 @@ def post(self, request, *args, **kwargs): new_tag = add_tag_to_taxonomy( taxonomy, tag, parent_tag_id, external_id ) - except Tag.DoesNotExist as e: + except TagDoesNotExist as e: raise Http404("Parent Tag not found") from e except ValueError as e: raise ValidationError(e) from e @@ -704,7 +705,7 @@ def update(self, request, *args, **kwargs): try: updated_tag = update_tag_in_taxonomy(taxonomy, tag, tag_value) - except Tag.DoesNotExist as e: + except TagDoesNotExist as e: raise Http404("Tag not found") from e except ValueError as e: raise ValidationError(e) from e diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index a32bda4b..2559ebc1 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -1210,6 +1210,19 @@ def test_next_children(self): assert data.get("num_pages") == 2 assert data.get("current_page") == 2 + def test_create_tag_in_taxonomy_while_loggedout(self): + new_tag_value = "New Tag" + + create_data = { + "tag": new_tag_value + } + + response = self.client.post( + self.small_taxonomy_url, create_data, format="json" + ) + + assert response.status_code == status.HTTP_401_UNAUTHORIZED + def test_create_tag_in_taxonomy(self): self.client.force_authenticate(user=self.user) new_tag_value = "New Tag" @@ -1366,6 +1379,24 @@ def test_create_tag_in_taxonomy_with_already_existing_value(self): assert response.status_code == status.HTTP_400_BAD_REQUEST + def test_update_tag_in_taxonomy_while_loggedout(self): + updated_tag_value = "Updated Tag" + + # Existing Tag that will be updated + existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() + + update_data = { + "tag": existing_tag.id, + "tag_value": updated_tag_value + } + + # Test updating using the PUT method + response = self.client.put( + self.small_taxonomy_url, update_data, format="json" + ) + + assert response.status_code == status.HTTP_401_UNAUTHORIZED + def test_update_tag_in_taxonomy_with_different_methods(self): self.client.force_authenticate(user=self.user) updated_tag_value = "Updated Tag" @@ -1528,6 +1559,21 @@ def test_update_tag_in_invalid_taxonomy(self): assert response.status_code == status.HTTP_404_NOT_FOUND + def test_delete_single_tag_from_taxonomy_while_loggedout(self): + # Get Tag that will be deleted + existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() + + delete_data = { + "tag_ids": [existing_tag.id], + "with_subtags": True + } + + response = self.client.delete( + self.small_taxonomy_url, delete_data, format="json" + ) + + assert response.status_code == status.HTTP_401_UNAUTHORIZED + def test_delete_single_tag_from_taxonomy(self): self.client.force_authenticate(user=self.user) From f09f75d16cbee3248497551b303be0b9390db75e Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Mon, 16 Oct 2023 18:07:46 +0300 Subject: [PATCH 05/13] feat: Update Taxonomy Tags permissions Utilize Taxonomy permissions to determine permissions of Taxonomy Tags --- .../core/tagging/rest_api/v1/permissions.py | 23 ++--- .../core/tagging/rest_api/v1/views.py | 26 +++++- openedx_tagging/core/tagging/rules.py | 29 ++++-- .../core/tagging/test_views.py | 91 ++++++++++++++----- 4 files changed, 125 insertions(+), 44 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/permissions.py b/openedx_tagging/core/tagging/rest_api/v1/permissions.py index ed184549..df2a7034 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/permissions.py +++ b/openedx_tagging/core/tagging/rest_api/v1/permissions.py @@ -35,20 +35,21 @@ class ObjectTagObjectPermissions(DjangoObjectPermissions): } -class TagListPermissions(DjangoObjectPermissions): +class TagObjectPermissions(DjangoObjectPermissions): """ - Permissions for Tag object views. + Maps each REST API methods to its corresponding Tag permission. """ - def has_permission(self, request, view): - """ - Returns True if the user on the given request is allowed the given view. - """ - if not request.user or ( - not request.user.is_authenticated and self.authenticated_users_only - ): - return False - return True + perms_map = { + "GET": ["%(app_label)s.view_%(model_name)s"], + "OPTIONS": [], + "HEAD": ["%(app_label)s.view_%(model_name)s"], + "POST": ["%(app_label)s.add_%(model_name)s"], + "PUT": ["%(app_label)s.change_%(model_name)s"], + "PATCH": ["%(app_label)s.change_%(model_name)s"], + "DELETE": ["%(app_label)s.delete_%(model_name)s"], + } + # This is to handle the special case for GET list of Taxonomy Tags def has_object_permission(self, request, view, obj): """ Returns True if the user on the given request is allowed the given view for the given object. diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index dc0c511e..663bd347 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -33,7 +33,7 @@ from ...models import Taxonomy from ...rules import ObjectTagPermissionItem from ..paginators import SEARCH_TAGS_THRESHOLD, TAGS_THRESHOLD, DisabledTagsPagination, TagsPagination -from .permissions import ObjectTagObjectPermissions, TagListPermissions, TaxonomyObjectPermissions +from .permissions import ObjectTagObjectPermissions, TagObjectPermissions, TaxonomyObjectPermissions from .serializers import ( ObjectTagListQueryParamsSerializer, ObjectTagSerializer, @@ -504,7 +504,7 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): """ - permission_classes = [TagListPermissions] + permission_classes = [TagObjectPermissions] pagination_enabled = True def __init__(self): @@ -637,7 +637,7 @@ def get_matching_tags( return result - def get_queryset(self) -> list[Tag]: # type: ignore[override] + def get_queryset(self) -> models.QuerySet[Tag]: # type: ignore[override] """ Builds and returns the queryset to be paginated. @@ -655,10 +655,26 @@ def get_queryset(self) -> list[Tag]: # type: ignore[override] search_term=search_term, ) + # Convert the results back to a QuerySet for permissions to apply + # Due to the conversion we lose the populated `sub_tags` attribute, + # in the case of using the special search serializer so we + # need to repopulate it again + if self.serializer_class == TagsForSearchSerializer: + results_dict = {tag.id: tag for tag in result} + + result_queryset = Tag.objects.filter(id__in=results_dict.keys()) + + for tag in result_queryset: + sub_tags = results_dict[tag.id].sub_tags # type: ignore[attr-defined] + tag.sub_tags = sub_tags # type: ignore[attr-defined] + + else: + result_queryset = Tag.objects.filter(id__in=[tag.id for tag in result]) + # This function is not called automatically self.pagination_class = self.get_pagination_class() - return result + return result_queryset def post(self, request, *args, **kwargs): """ @@ -683,6 +699,7 @@ def post(self, request, *args, **kwargs): except ValueError as e: raise ValidationError(e) from e + self.serializer_class = TagsSerializer serializer_context = self.get_serializer_context() return Response( self.serializer_class(new_tag, context=serializer_context).data, @@ -710,6 +727,7 @@ def update(self, request, *args, **kwargs): except ValueError as e: raise ValidationError(e) from e + self.serializer_class = TagsSerializer serializer_context = self.get_serializer_context() return Response( self.serializer_class(updated_tag, context=serializer_context).data, diff --git a/openedx_tagging/core/tagging/rules.py b/openedx_tagging/core/tagging/rules.py index 8249628f..bb22b2de 100644 --- a/openedx_tagging/core/tagging/rules.py +++ b/openedx_tagging/core/tagging/rules.py @@ -51,17 +51,28 @@ def can_change_taxonomy(user: UserType, taxonomy: Taxonomy | None = None) -> boo ) +@rules.predicate +def can_view_tag(user: UserType, tag: Tag | None = None) -> bool: + """ + User can view tags for any taxonomy they can view. + """ + taxonomy = tag.taxonomy.cast() if (tag and tag.taxonomy) else None + has_perm_thing = user.has_perm( + "oel_tagging.view_taxonomy", + taxonomy, + ) + return has_perm_thing + + @rules.predicate def can_change_tag(user: UserType, tag: Tag | None = None) -> bool: """ - Even taxonomy admins cannot add tags to system taxonomies (their tags are system-defined), or free-text taxonomies - (these don't have predefined tags). + Users can change tags for any taxonomy they can modify. """ taxonomy = tag.taxonomy.cast() if (tag and tag.taxonomy) else None - return is_taxonomy_admin(user) and ( - not tag - or not taxonomy - or (taxonomy and not taxonomy.allow_free_text and not taxonomy.system_defined) + return user.has_perm( + "oel_tagging.change_taxonomy", + taxonomy, ) @@ -166,8 +177,10 @@ def can_change_object_tag( # Tag rules.add_perm("oel_tagging.add_tag", can_change_tag) rules.add_perm("oel_tagging.change_tag", can_change_tag) -rules.add_perm("oel_tagging.delete_tag", is_taxonomy_admin) -rules.add_perm("oel_tagging.view_tag", rules.always_allow) +rules.add_perm("oel_tagging.delete_tag", can_change_tag) +rules.add_perm("oel_tagging.view_tag", can_view_tag) +# Special Case for listing Tags, we check if we can view the Taxonomy since +# that is what is passed in rather than a Tag object rules.add_perm("oel_tagging.list_tag", can_view_taxonomy) # ObjectTag diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 2559ebc1..1d5cf2b8 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -1223,7 +1223,7 @@ def test_create_tag_in_taxonomy_while_loggedout(self): assert response.status_code == status.HTTP_401_UNAUTHORIZED - def test_create_tag_in_taxonomy(self): + def test_create_tag_in_taxonomy_without_permission(self): self.client.force_authenticate(user=self.user) new_tag_value = "New Tag" @@ -1235,6 +1235,20 @@ def test_create_tag_in_taxonomy(self): self.small_taxonomy_url, create_data, format="json" ) + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_create_tag_in_taxonomy(self): + self.client.force_authenticate(user=self.staff) + new_tag_value = "New Tag" + + create_data = { + "tag": new_tag_value + } + + response = self.client.post( + self.small_taxonomy_url, create_data, format="json" + ) + assert response.status_code == status.HTTP_201_CREATED data = response.data @@ -1248,7 +1262,7 @@ def test_create_tag_in_taxonomy(self): self.assertEqual(data.get("children_count"), 0) def test_create_tag_in_taxonomy_with_parent_id(self): - self.client.force_authenticate(user=self.user) + self.client.force_authenticate(user=self.staff) parent_tag = self.small_taxonomy.tag_set.filter(parent=None).first() new_tag_value = "New Child Tag" new_external_id = "extId" @@ -1276,7 +1290,7 @@ def test_create_tag_in_taxonomy_with_parent_id(self): self.assertEqual(data.get("children_count"), 0) def test_create_tag_in_invalid_taxonomy(self): - self.client.force_authenticate(user=self.user) + self.client.force_authenticate(user=self.staff) new_tag_value = "New Tag" create_data = { @@ -1291,7 +1305,7 @@ def test_create_tag_in_invalid_taxonomy(self): assert response.status_code == status.HTTP_404_NOT_FOUND def test_create_tag_in_free_text_taxonomy(self): - self.client.force_authenticate(user=self.user) + self.client.force_authenticate(user=self.staff) new_tag_value = "New Tag" create_data = { @@ -1309,7 +1323,7 @@ def test_create_tag_in_free_text_taxonomy(self): assert response.status_code == status.HTTP_400_BAD_REQUEST def test_create_tag_in_system_defined_taxonomy(self): - self.client.force_authenticate(user=self.user) + self.client.force_authenticate(user=self.staff) new_tag_value = "New Tag" create_data = { @@ -1327,7 +1341,7 @@ def test_create_tag_in_system_defined_taxonomy(self): assert response.status_code == status.HTTP_400_BAD_REQUEST def test_create_tag_in_taxonomy_with_invalid_parent_tag_id(self): - self.client.force_authenticate(user=self.user) + self.client.force_authenticate(user=self.staff) invalid_parent_tag_id = 91919 new_tag_value = "New Child Tag" @@ -1343,7 +1357,7 @@ def test_create_tag_in_taxonomy_with_invalid_parent_tag_id(self): assert response.status_code == status.HTTP_400_BAD_REQUEST def test_create_tag_in_taxonomy_with_parent_tag_id_in_other_taxonomy(self): - self.client.force_authenticate(user=self.user) + self.client.force_authenticate(user=self.staff) invalid_parent_tag_id = 1 new_tag_value = "New Child Tag" @@ -1359,7 +1373,7 @@ def test_create_tag_in_taxonomy_with_parent_tag_id_in_other_taxonomy(self): assert response.status_code == status.HTTP_404_NOT_FOUND def test_create_tag_in_taxonomy_with_already_existing_value(self): - self.client.force_authenticate(user=self.user) + self.client.force_authenticate(user=self.staff) new_tag_value = "New Tag" create_data = { @@ -1397,9 +1411,28 @@ def test_update_tag_in_taxonomy_while_loggedout(self): assert response.status_code == status.HTTP_401_UNAUTHORIZED - def test_update_tag_in_taxonomy_with_different_methods(self): + def test_update_tag_in_taxonomy_without_permission(self): self.client.force_authenticate(user=self.user) updated_tag_value = "Updated Tag" + + # Existing Tag that will be updated + existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() + + update_data = { + "tag": existing_tag.id, + "tag_value": updated_tag_value + } + + # Test updating using the PUT method + response = self.client.put( + self.small_taxonomy_url, update_data, format="json" + ) + + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_update_tag_in_taxonomy_with_different_methods(self): + self.client.force_authenticate(user=self.staff) + updated_tag_value = "Updated Tag" updated_tag_value_2 = "Updated Tag 2" # Existing Tag that will be updated @@ -1444,7 +1477,7 @@ def test_update_tag_in_taxonomy_with_different_methods(self): self.assertEqual(data.get("external_id"), existing_tag.external_id) def test_update_tag_in_taxonomy_reflects_changes_in_object_tags(self): - self.client.force_authenticate(user=self.user) + self.client.force_authenticate(user=self.staff) existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() @@ -1495,7 +1528,7 @@ def test_update_tag_in_taxonomy_reflects_changes_in_object_tags(self): self.assertEqual(object_tag_3.value, updated_tag_value) def test_update_tag_in_taxonomy_with_invalid_tag_id(self): - self.client.force_authenticate(user=self.user) + self.client.force_authenticate(user=self.staff) updated_tag_value = "Updated Tag" update_data = { @@ -1510,7 +1543,7 @@ def test_update_tag_in_taxonomy_with_invalid_tag_id(self): assert response.status_code == status.HTTP_400_BAD_REQUEST def test_update_tag_in_taxonomy_with_tag_id_in_other_taxonomy(self): - self.client.force_authenticate(user=self.user) + self.client.force_authenticate(user=self.staff) updated_tag_value = "Updated Tag" update_data = { @@ -1525,7 +1558,7 @@ def test_update_tag_in_taxonomy_with_tag_id_in_other_taxonomy(self): assert response.status_code == status.HTTP_404_NOT_FOUND def test_update_tag_in_taxonomy_with_no_tag_value_provided(self): - self.client.force_authenticate(user=self.user) + self.client.force_authenticate(user=self.staff) # Existing Tag that will be updated existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() @@ -1541,7 +1574,7 @@ def test_update_tag_in_taxonomy_with_no_tag_value_provided(self): assert response.status_code == status.HTTP_400_BAD_REQUEST def test_update_tag_in_invalid_taxonomy(self): - self.client.force_authenticate(user=self.user) + self.client.force_authenticate(user=self.staff) # Existing Tag that will be updated existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() @@ -1574,8 +1607,24 @@ def test_delete_single_tag_from_taxonomy_while_loggedout(self): assert response.status_code == status.HTTP_401_UNAUTHORIZED - def test_delete_single_tag_from_taxonomy(self): + def test_delete_single_tag_from_taxonomy_without_permission(self): self.client.force_authenticate(user=self.user) + # Get Tag that will be deleted + existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() + + delete_data = { + "tag_ids": [existing_tag.id], + "with_subtags": True + } + + response = self.client.delete( + self.small_taxonomy_url, delete_data, format="json" + ) + + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_delete_single_tag_from_taxonomy(self): + self.client.force_authenticate(user=self.staff) # Get Tag that will be deleted existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() @@ -1596,7 +1645,7 @@ def test_delete_single_tag_from_taxonomy(self): existing_tag.refresh_from_db() def test_delete_multiple_tags_from_taxonomy(self): - self.client.force_authenticate(user=self.user) + self.client.force_authenticate(user=self.staff) # Get Tags that will be deleted existing_tags = self.small_taxonomy.tag_set.filter(parent=None)[:3] @@ -1618,7 +1667,7 @@ def test_delete_multiple_tags_from_taxonomy(self): existing_tag.refresh_from_db() def test_delete_tag_with_subtags_should_fail_without_flag_passed(self): - self.client.force_authenticate(user=self.user) + self.client.force_authenticate(user=self.staff) # Get Tag that will be deleted existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() @@ -1634,7 +1683,7 @@ def test_delete_tag_with_subtags_should_fail_without_flag_passed(self): assert response.status_code == status.HTTP_400_BAD_REQUEST def test_delete_tag_in_invalid_taxonomy(self): - self.client.force_authenticate(user=self.user) + self.client.force_authenticate(user=self.staff) # Get Tag that will be deleted existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() @@ -1651,7 +1700,7 @@ def test_delete_tag_in_invalid_taxonomy(self): assert response.status_code == status.HTTP_404_NOT_FOUND def test_delete_tag_in_taxonomy_with_invalid_tag_id(self): - self.client.force_authenticate(user=self.user) + self.client.force_authenticate(user=self.staff) delete_data = { "tag_ids": [91919] @@ -1664,7 +1713,7 @@ def test_delete_tag_in_taxonomy_with_invalid_tag_id(self): assert response.status_code == status.HTTP_400_BAD_REQUEST def test_delete_tag_with_tag_id_in_other_taxonomy(self): - self.client.force_authenticate(user=self.user) + self.client.force_authenticate(user=self.staff) # Get Tag in other Taxonomy tag_in_other_taxonomy = self.small_taxonomy.tag_set.filter(parent=None).first() @@ -1680,7 +1729,7 @@ def test_delete_tag_with_tag_id_in_other_taxonomy(self): assert response.status_code == status.HTTP_400_BAD_REQUEST def test_delete_tag_in_taxonomy_without_subtags(self): - self.client.force_authenticate(user=self.user) + self.client.force_authenticate(user=self.staff) # Get Tag that will be deleted existing_tag = self.small_taxonomy.tag_set.filter(children__isnull=True).first() From f9e28daf0093ef7ceb15a68f354f30945ff2c108 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Tue, 17 Oct 2023 13:49:41 +0300 Subject: [PATCH 06/13] fix: Bug DRF UI when creating tag + failing test --- openedx_tagging/core/tagging/models/base.py | 1 - openedx_tagging/core/tagging/rest_api/v1/permissions.py | 3 +++ openedx_tagging/core/tagging/rules.py | 3 +-- tests/openedx_tagging/core/tagging/test_rules.py | 4 ++-- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index 5fcdcb33..d23cb8a4 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -347,7 +347,6 @@ def add_tag( tag_value: str, parent_tag_id: int | None = None, external_id: str | None = None - ) -> Tag: """ Add new Tag to Taxonomy. If an existing Tag with the `tag_value` already diff --git a/openedx_tagging/core/tagging/rest_api/v1/permissions.py b/openedx_tagging/core/tagging/rest_api/v1/permissions.py index df2a7034..b63a6b7e 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/permissions.py +++ b/openedx_tagging/core/tagging/rest_api/v1/permissions.py @@ -4,6 +4,8 @@ import rules # type: ignore[import] from rest_framework.permissions import DjangoObjectPermissions +from ...models import Tag + class TaxonomyObjectPermissions(DjangoObjectPermissions): """ @@ -54,4 +56,5 @@ def has_object_permission(self, request, view, obj): """ Returns True if the user on the given request is allowed the given view for the given object. """ + obj = obj.taxonomy if isinstance(obj, Tag) else obj return rules.has_perm("oel_tagging.list_tag", request.user, obj) diff --git a/openedx_tagging/core/tagging/rules.py b/openedx_tagging/core/tagging/rules.py index bb22b2de..97ca56de 100644 --- a/openedx_tagging/core/tagging/rules.py +++ b/openedx_tagging/core/tagging/rules.py @@ -57,11 +57,10 @@ def can_view_tag(user: UserType, tag: Tag | None = None) -> bool: User can view tags for any taxonomy they can view. """ taxonomy = tag.taxonomy.cast() if (tag and tag.taxonomy) else None - has_perm_thing = user.has_perm( + return user.has_perm( "oel_tagging.view_taxonomy", taxonomy, ) - return has_perm_thing @rules.predicate diff --git a/tests/openedx_tagging/core/tagging/test_rules.py b/tests/openedx_tagging/core/tagging/test_rules.py index f30cbd27..11cc2ced 100644 --- a/tests/openedx_tagging/core/tagging/test_rules.py +++ b/tests/openedx_tagging/core/tagging/test_rules.py @@ -141,12 +141,12 @@ def test_add_change_tag(self, perm): ) def test_tag_free_text_taxonomy(self, perm): """ - Taxonomy administrators cannot modify tags on a free-text Taxonomy + Taxonomy administrators can modify any Tag, even those associated with a free-text Taxonomy """ self.taxonomy.allow_free_text = True self.taxonomy.save() assert self.superuser.has_perm(perm, self.bacteria) - assert not self.staff.has_perm(perm, self.bacteria) + assert self.staff.has_perm(perm, self.bacteria) assert not self.learner.has_perm(perm, self.bacteria) @ddt.data( From bdaffeb0d0e718ce56321be6db9a64b83e64bc47 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Tue, 17 Oct 2023 14:36:08 +0300 Subject: [PATCH 07/13] refactor: Perform resync_object_tags in api --- openedx_tagging/core/tagging/api.py | 27 ++++++++++++++++++--- openedx_tagging/core/tagging/models/base.py | 19 --------------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index 19d4f11e..2a7bff9a 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -129,7 +129,14 @@ def resync_object_tags(object_tags: QuerySet | None = None) -> int: if not object_tags: object_tags = ObjectTag.objects.select_related("tag", "taxonomy") - return ObjectTag.resync_object_tags(object_tags) + num_changed = 0 + for object_tag in object_tags: + changed = object_tag.resync() + if changed: + object_tag.save() + num_changed += 1 + + return num_changed def get_object_tags( @@ -332,12 +339,19 @@ def update_tag_in_taxonomy(taxonomy: Taxonomy, tag: int, tag_value: str): Currently only support updates the Tag value. """ - return taxonomy.cast().update_tag(tag, tag_value) + taxonomy = taxonomy.cast() + updated_tag = taxonomy.update_tag(tag, tag_value) + + # Resync all related ObjectTags to update to the new Tag value + object_tags = taxonomy.objecttag_set.all() + resync_object_tags(object_tags) + + return updated_tag def delete_tags_from_taxonomy( taxonomy: Taxonomy, - tag_ids: list[Tag], + tag_ids: list[int], with_subtags: bool ): """ @@ -345,4 +359,9 @@ def delete_tags_from_taxonomy( the `with_subtags` is not set to `True` it will fail, otherwise the sub-tags will be deleted as well. """ - return taxonomy.cast().delete_tags(tag_ids, with_subtags) + taxonomy = taxonomy.cast() + taxonomy.delete_tags(tag_ids, with_subtags) + + # Resync all related ObjectTags after deleting the Tag(s) + object_tags = taxonomy.objecttag_set.all() + resync_object_tags(object_tags) diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index d23cb8a4..cbf224b4 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -399,10 +399,6 @@ def update_tag(self, tag_id: int, tag_value: str) -> Tag: tag = self.tag_set.get(id=tag_id) tag.value = tag_value tag.save() - - # Resync all related ObjectTags to update to the new Tag value - object_tags = self.objecttag_set.all() - ObjectTag.resync_object_tags(object_tags) return tag def delete_tags(self, tag_ids: List[int], with_subtags: bool = False): @@ -709,18 +705,3 @@ def copy(self, object_tag: ObjectTag) -> Self: self._value = object_tag._value # pylint: disable=protected-access self._name = object_tag._name # pylint: disable=protected-access return self - - @classmethod - def resync_object_tags(cls, object_tags: models.QuerySet[ObjectTag]) -> int: - """ - Reconciles ObjectTag entries with any changes made to their associated - taxonomies and tags. Return the number of changes made. - """ - num_changed = 0 - for object_tag in object_tags: - changed = object_tag.resync() - if changed: - object_tag.save() - num_changed += 1 - - return num_changed From b2e5323ac9954f0f3ca33975b3d1d6614e3134e5 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Tue, 17 Oct 2023 17:06:04 +0300 Subject: [PATCH 08/13] refactor: Use Taxonomy Tag values rather than IDs --- openedx_tagging/core/tagging/api.py | 14 ++-- openedx_tagging/core/tagging/models/base.py | 26 +++---- .../core/tagging/rest_api/v1/serializers.py | 49 +++++++++--- .../core/tagging/rest_api/v1/views.py | 33 +++++---- .../core/tagging/test_views.py | 74 ++++++++++--------- 5 files changed, 113 insertions(+), 83 deletions(-) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index 2a7bff9a..ada76588 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -321,7 +321,7 @@ def autocomplete_tags( def add_tag_to_taxonomy( taxonomy: Taxonomy, tag: str, - parent_tag_id: int | None = None, + parent_tag_value: str | None = None, external_id: str | None = None ) -> Tag: """ @@ -329,18 +329,18 @@ def add_tag_to_taxonomy( Taxonomy, an exception is raised, otherwise the newly created Tag is returned """ - return taxonomy.cast().add_tag(tag, parent_tag_id, external_id) + return taxonomy.cast().add_tag(tag, parent_tag_value, external_id) -def update_tag_in_taxonomy(taxonomy: Taxonomy, tag: int, tag_value: str): +def update_tag_in_taxonomy(taxonomy: Taxonomy, tag: str, new_value: str): """ Update a Tag that belongs to a Taxonomy. The related ObjectTags are updated accordingly. - Currently only support updates the Tag value. + Currently only supports updating the Tag value. """ taxonomy = taxonomy.cast() - updated_tag = taxonomy.update_tag(tag, tag_value) + updated_tag = taxonomy.update_tag(tag, new_value) # Resync all related ObjectTags to update to the new Tag value object_tags = taxonomy.objecttag_set.all() @@ -351,7 +351,7 @@ def update_tag_in_taxonomy(taxonomy: Taxonomy, tag: int, tag_value: str): def delete_tags_from_taxonomy( taxonomy: Taxonomy, - tag_ids: list[int], + tags: list[str], with_subtags: bool ): """ @@ -360,7 +360,7 @@ def delete_tags_from_taxonomy( the sub-tags will be deleted as well. """ taxonomy = taxonomy.cast() - taxonomy.delete_tags(tag_ids, with_subtags) + taxonomy.delete_tags(tags, with_subtags) # Resync all related ObjectTags after deleting the Tag(s) object_tags = taxonomy.objecttag_set.all() diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index cbf224b4..ebbe5b68 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -345,7 +345,7 @@ def get_filtered_tags( def add_tag( self, tag_value: str, - parent_tag_id: int | None = None, + parent_tag_value: str | None = None, external_id: str | None = None ) -> Tag: """ @@ -369,8 +369,8 @@ def add_tag( raise ValueError(f"Tag with value '{tag_value}' already exists for taxonomy.") parent = None - if parent_tag_id: - parent = self.tag_set.get(id=parent_tag_id) + if parent_tag_value: + parent = self.tag_set.get(value__iexact=parent_tag_value) tag = Tag.objects.create( taxonomy=self, value=tag_value, parent=parent, external_id=external_id @@ -378,7 +378,7 @@ def add_tag( return tag - def update_tag(self, tag_id: int, tag_value: str) -> Tag: + def update_tag(self, tag: str, new_value: str) -> Tag: """ Update an existing Tag in Taxonomy and return it. Currently only supports updating the Tag's value. @@ -396,12 +396,12 @@ def update_tag(self, tag_id: int, tag_value: str) -> Tag: ) # Update Tag instance with new value - tag = self.tag_set.get(id=tag_id) - tag.value = tag_value - tag.save() - return tag + tag_to_update = self.tag_set.get(value__iexact=tag) + tag_to_update.value = new_value + tag_to_update.save() + return tag_to_update - def delete_tags(self, tag_ids: List[int], with_subtags: bool = False): + def delete_tags(self, tags: List[str], with_subtags: bool = False): """ Delete the Taxonomy Tags provided. If any of them have children and the `with_subtags` is not set to `True` it will fail, otherwise @@ -419,15 +419,15 @@ def delete_tags(self, tag_ids: List[int], with_subtags: bool = False): "delete_tags() doesn't work for system defined taxonomies. They cannot be modified." ) - tags = self.tag_set.filter(id__in=tag_ids) + tags_to_delete = self.tag_set.filter(value__in=tags) - if tags.count() != len(tag_ids): + if tags_to_delete.count() != len(tags): # If they do not match that means there is a Tag ID in the provided # list that is either invalid or does not belong to this Taxonomy raise ValueError("Invalid tag id provided or tag id does not belong to taxonomy") # Check if any Tag contains subtags (children) - contains_children = tags.filter(children__isnull=False).distinct().exists() + contains_children = tags_to_delete.filter(children__isnull=False).distinct().exists() if contains_children and not with_subtags: raise ValueError( @@ -436,7 +436,7 @@ def delete_tags(self, tag_ids: List[int], with_subtags: bool = False): ) # Delete the Tags with their subtags if any - tags.delete() + tags_to_delete.delete() def validate_value(self, value: str) -> bool: """ diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index 0a1ef3cb..832a371d 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -1,7 +1,6 @@ """ API Serializers for taxonomies """ - from rest_framework import serializers from rest_framework.reverse import reverse @@ -201,21 +200,40 @@ class TaxonomyTagCreateBodySerializer(serializers.Serializer): # pylint: disabl """ tag = serializers.CharField(required=True) - parent_tag_id = serializers.PrimaryKeyRelatedField( - queryset=Tag.objects.all(), required=False + parent_tag_value = serializers.CharField( + source='parent.value', required=False ) external_id = serializers.CharField(required=False) + def validate_parent_tag_value(self, value): + """ + Check that the provided parent Tag exists based on the value + """ + valid = Tag.objects.filter(value__iexact=value).exists() + if not valid: + raise serializers.ValidationError("Invalid `parent_tag_value` provided") + + return value + class TaxonomyTagUpdateBodySerializer(serializers.Serializer): # pylint: disable=abstract-method """ Serializer of the body for the Taxonomy Tags UPDATE view """ - tag = serializers.PrimaryKeyRelatedField( - queryset=Tag.objects.all(), required=True - ) - tag_value = serializers.CharField(required=True) + tag = serializers.CharField(source="value", required=True) + updated_tag_value = serializers.CharField(required=True) + + def validate_tag(self, value): + """ + Check that the provided Tag exists based on the value + """ + + valid = Tag.objects.filter(value__iexact=value).exists() + if not valid: + raise serializers.ValidationError("Invalid `tag` provided") + + return value class TaxonomyTagDeleteBodySerializer(serializers.Serializer): # pylint: disable=abstract-method @@ -223,9 +241,18 @@ class TaxonomyTagDeleteBodySerializer(serializers.Serializer): # pylint: disabl Serializer of the body for the Taxonomy Tags DELETE view """ - tag_ids = serializers.PrimaryKeyRelatedField( - queryset=Tag.objects.all(), - many=True, - required=True + tags = serializers.ListField( + child=serializers.CharField(), required=True ) with_subtags = serializers.BooleanField(required=False) + + def validate_tags(self, value): + """ + Check that the provided Tags exists based on the values + """ + + valid = Tag.objects.filter(value__in=value).count() == len(value) + if not valid: + raise serializers.ValidationError("One or more tag in `tags` is invalid") + + return value diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index 663bd347..abeb4152 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -437,7 +437,7 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): **Create Request Body** * tag (required): The value of the Tag that should be added to the Taxonomy - * parent_tag_id (optional): The id of the parent tag that the new + * parent_tag_value (optional): The value of the parent tag that the new Tag should fall under * extenal_id (optional): The external id for the new Tag @@ -445,7 +445,7 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): POST api/tagging/v1/taxonomy/:pk/tags - Create a Tag in taxonomy { "value": "New Tag", - "parent_tag_id": 123 + "parent_tag_value": "Parent Tag" "external_id": "abc123", } @@ -459,19 +459,19 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): * pk (required) - The pk of the taxonomy to update a Tag in **Update Request Body** - * tag (required): The ID of the Tag that should be updated - * tag_value (required): The updated value of the Tag + * tag (required): The value (identifier) of the Tag to be updated + * updated_tag_value (required): The updated value of the Tag **Update Example Requests** PUT api/tagging/v1/taxonomy/:pk/tags - Update a Tag in Taxonomy { - "tag": 1, - "tag_value": "Updated Tag Value" + "tag": "Tag 1", + "updated_tag_value": "Updated Tag Value" } PATCH api/tagging/v1/taxonomy/:pk/tags - Update a Tag in Taxonomy { - "tag": 1, - "tag_value": "Updated Tag Value" + "tag": "Tag 1", + "updated_tag_value": "Updated Tag Value" } **Update Query Returns** @@ -484,7 +484,8 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): * pk (required) - The pk of the taxonomy to Delete Tag(s) in **Delete Request Body** - * tag_ids (required): The IDs of Tags that should be deleted from Taxonomy + * tags (required): The values (identifiers) of Tags that should be + deleted from Taxonomy * with_subtags (optional): If a Tag in the provided ids contains children (subtags), deletion will fail unless set to `True`. Defaults to `False`. @@ -492,7 +493,7 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): **Delete Example Requests** DELETE api/tagging/v1/taxonomy/:pk/tags - Delete Tag(s) in Taxonomy { - "tag_ids": [1,2,3], + "tags": ["Tag 1", "Tag 2", "Tag 3"], "with_subtags": True } @@ -687,12 +688,12 @@ def post(self, request, *args, **kwargs): body.is_valid(raise_exception=True) tag = body.data.get("tag") - parent_tag_id = body.data.get("parent_tag_id", None) + parent_tag_value = body.data.get("parent_tag_value", None) external_id = body.data.get("external_id", None) try: new_tag = add_tag_to_taxonomy( - taxonomy, tag, parent_tag_id, external_id + taxonomy, tag, parent_tag_value, external_id ) except TagDoesNotExist as e: raise Http404("Parent Tag not found") from e @@ -718,10 +719,10 @@ def update(self, request, *args, **kwargs): body.is_valid(raise_exception=True) tag = body.data.get("tag") - tag_value = body.data.get("tag_value") + updated_tag_value = body.data.get("updated_tag_value") try: - updated_tag = update_tag_in_taxonomy(taxonomy, tag, tag_value) + updated_tag = update_tag_in_taxonomy(taxonomy, tag, updated_tag_value) except TagDoesNotExist as e: raise Http404("Tag not found") from e except ValueError as e: @@ -746,11 +747,11 @@ def delete(self, request, *args, **kwargs): body = TaxonomyTagDeleteBodySerializer(data=request.data) body.is_valid(raise_exception=True) - tag_ids = body.data.get("tag_ids") + tags = body.data.get("tags") with_subtags = body.data.get("with_subtags") try: - delete_tags_from_taxonomy(taxonomy, tag_ids, with_subtags) + delete_tags_from_taxonomy(taxonomy, tags, with_subtags) except ValueError as e: raise ValidationError(e) from e diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 1d5cf2b8..5274f7d1 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -1261,7 +1261,7 @@ def test_create_tag_in_taxonomy(self): self.assertIsNone(data.get("sub_tags_link")) self.assertEqual(data.get("children_count"), 0) - def test_create_tag_in_taxonomy_with_parent_id(self): + def test_create_tag_in_taxonomy_with_parent(self): self.client.force_authenticate(user=self.staff) parent_tag = self.small_taxonomy.tag_set.filter(parent=None).first() new_tag_value = "New Child Tag" @@ -1269,7 +1269,7 @@ def test_create_tag_in_taxonomy_with_parent_id(self): create_data = { "tag": new_tag_value, - "parent_tag_id": parent_tag.id, + "parent_tag_value": parent_tag.value, "external_id": new_external_id } @@ -1340,14 +1340,14 @@ def test_create_tag_in_system_defined_taxonomy(self): assert response.status_code == status.HTTP_400_BAD_REQUEST - def test_create_tag_in_taxonomy_with_invalid_parent_tag_id(self): + def test_create_tag_in_taxonomy_with_invalid_parent_tag(self): self.client.force_authenticate(user=self.staff) - invalid_parent_tag_id = 91919 + invalid_parent_tag = "Invalid Tag" new_tag_value = "New Child Tag" create_data = { "tag": new_tag_value, - "parent_tag_id": invalid_parent_tag_id, + "parent_tag_value": invalid_parent_tag, } response = self.client.post( @@ -1356,14 +1356,14 @@ def test_create_tag_in_taxonomy_with_invalid_parent_tag_id(self): assert response.status_code == status.HTTP_400_BAD_REQUEST - def test_create_tag_in_taxonomy_with_parent_tag_id_in_other_taxonomy(self): + def test_create_tag_in_taxonomy_with_parent_tag_in_other_taxonomy(self): self.client.force_authenticate(user=self.staff) - invalid_parent_tag_id = 1 + tag_in_other_taxonomy = Tag.objects.get(id=1) new_tag_value = "New Child Tag" create_data = { "tag": new_tag_value, - "parent_tag_id": invalid_parent_tag_id, + "parent_tag_value": tag_in_other_taxonomy.value, } response = self.client.post( @@ -1400,8 +1400,8 @@ def test_update_tag_in_taxonomy_while_loggedout(self): existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() update_data = { - "tag": existing_tag.id, - "tag_value": updated_tag_value + "tag": existing_tag.value, + "updated_tag_value": updated_tag_value } # Test updating using the PUT method @@ -1419,8 +1419,8 @@ def test_update_tag_in_taxonomy_without_permission(self): existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() update_data = { - "tag": existing_tag.id, - "tag_value": updated_tag_value + "tag": existing_tag.value, + "updated_tag_value": updated_tag_value } # Test updating using the PUT method @@ -1439,8 +1439,8 @@ def test_update_tag_in_taxonomy_with_different_methods(self): existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() update_data = { - "tag": existing_tag.id, - "tag_value": updated_tag_value + "tag": existing_tag.value, + "updated_tag_value": updated_tag_value } # Test updating using the PUT method @@ -1460,7 +1460,8 @@ def test_update_tag_in_taxonomy_with_different_methods(self): self.assertEqual(data.get("external_id"), existing_tag.external_id) # Test updating using the PATCH method - update_data["tag_value"] = updated_tag_value_2 + update_data["tag"] = updated_tag_value # Since the value changed + update_data["updated_tag_value"] = updated_tag_value_2 response = self.client.patch( self.small_taxonomy_url, update_data, format="json" ) @@ -1499,8 +1500,8 @@ def test_update_tag_in_taxonomy_reflects_changes_in_object_tags(self): updated_tag_value = "Updated Tag" update_data = { - "tag": existing_tag.id, - "tag_value": updated_tag_value + "tag": existing_tag.value, + "updated_tag_value": updated_tag_value } # Test updating using the PUT method @@ -1527,13 +1528,13 @@ def test_update_tag_in_taxonomy_reflects_changes_in_object_tags(self): object_tag_3.refresh_from_db() self.assertEqual(object_tag_3.value, updated_tag_value) - def test_update_tag_in_taxonomy_with_invalid_tag_id(self): + def test_update_tag_in_taxonomy_with_invalid_tag(self): self.client.force_authenticate(user=self.staff) updated_tag_value = "Updated Tag" update_data = { "tag": 919191, - "tag_value": updated_tag_value + "updated_tag_value": updated_tag_value } response = self.client.put( @@ -1542,13 +1543,14 @@ def test_update_tag_in_taxonomy_with_invalid_tag_id(self): assert response.status_code == status.HTTP_400_BAD_REQUEST - def test_update_tag_in_taxonomy_with_tag_id_in_other_taxonomy(self): + def test_update_tag_in_taxonomy_with_tag_in_other_taxonomy(self): self.client.force_authenticate(user=self.staff) updated_tag_value = "Updated Tag" + tag_in_other_taxonomy = Tag.objects.get(id=1) update_data = { - "tag": 1, - "tag_value": updated_tag_value + "tag": tag_in_other_taxonomy.value, + "updated_tag_value": updated_tag_value } response = self.client.put( @@ -1564,7 +1566,7 @@ def test_update_tag_in_taxonomy_with_no_tag_value_provided(self): existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() update_data = { - "tag": existing_tag.id + "tag": existing_tag.value } response = self.client.put( @@ -1581,8 +1583,8 @@ def test_update_tag_in_invalid_taxonomy(self): updated_tag_value = "Updated Tag" update_data = { - "tag": existing_tag.id, - "tag_value": updated_tag_value + "tag": existing_tag.value, + "updated_tag_value": updated_tag_value } invalid_taxonomy_url = TAXONOMY_TAGS_URL.format(pk=919191) @@ -1597,7 +1599,7 @@ def test_delete_single_tag_from_taxonomy_while_loggedout(self): existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() delete_data = { - "tag_ids": [existing_tag.id], + "tags": [existing_tag.value], "with_subtags": True } @@ -1613,7 +1615,7 @@ def test_delete_single_tag_from_taxonomy_without_permission(self): existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() delete_data = { - "tag_ids": [existing_tag.id], + "tags": [existing_tag.value], "with_subtags": True } @@ -1630,7 +1632,7 @@ def test_delete_single_tag_from_taxonomy(self): existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() delete_data = { - "tag_ids": [existing_tag.id], + "tags": [existing_tag.value], "with_subtags": True } @@ -1651,7 +1653,7 @@ def test_delete_multiple_tags_from_taxonomy(self): existing_tags = self.small_taxonomy.tag_set.filter(parent=None)[:3] delete_data = { - "tag_ids": [existing_tag.id for existing_tag in existing_tags], + "tags": [existing_tag.value for existing_tag in existing_tags], "with_subtags": True } @@ -1673,7 +1675,7 @@ def test_delete_tag_with_subtags_should_fail_without_flag_passed(self): existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() delete_data = { - "tag_ids": [existing_tag.id] + "tags": [existing_tag.value] } response = self.client.delete( @@ -1689,7 +1691,7 @@ def test_delete_tag_in_invalid_taxonomy(self): existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() delete_data = { - "tag_ids": [existing_tag.id] + "tags": [existing_tag.value] } invalid_taxonomy_url = TAXONOMY_TAGS_URL.format(pk=919191) @@ -1699,11 +1701,11 @@ def test_delete_tag_in_invalid_taxonomy(self): assert response.status_code == status.HTTP_404_NOT_FOUND - def test_delete_tag_in_taxonomy_with_invalid_tag_id(self): + def test_delete_tag_in_taxonomy_with_invalid_tag(self): self.client.force_authenticate(user=self.staff) delete_data = { - "tag_ids": [91919] + "tags": ["Invalid Tag"] } response = self.client.delete( @@ -1712,14 +1714,14 @@ def test_delete_tag_in_taxonomy_with_invalid_tag_id(self): assert response.status_code == status.HTTP_400_BAD_REQUEST - def test_delete_tag_with_tag_id_in_other_taxonomy(self): + def test_delete_tag_with_tag_in_other_taxonomy(self): self.client.force_authenticate(user=self.staff) # Get Tag in other Taxonomy tag_in_other_taxonomy = self.small_taxonomy.tag_set.filter(parent=None).first() delete_data = { - "tag_ids": [tag_in_other_taxonomy.id] + "tags": [tag_in_other_taxonomy.value] } response = self.client.delete( @@ -1735,7 +1737,7 @@ def test_delete_tag_in_taxonomy_without_subtags(self): existing_tag = self.small_taxonomy.tag_set.filter(children__isnull=True).first() delete_data = { - "tag_ids": [existing_tag.id] + "tags": [existing_tag.value] } response = self.client.delete( From dd35747d6f4d7f4c11057800a206a69186948890 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Wed, 18 Oct 2023 13:00:38 +0300 Subject: [PATCH 09/13] fix: Move resync from delete_tags to add_tags --- openedx_tagging/core/tagging/api.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index ada76588..3316b32b 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -329,7 +329,16 @@ def add_tag_to_taxonomy( Taxonomy, an exception is raised, otherwise the newly created Tag is returned """ - return taxonomy.cast().add_tag(tag, parent_tag_value, external_id) + taxonomy = taxonomy.cast() + new_tag = taxonomy.add_tag(tag, parent_tag_value, external_id) + + # Resync all related ObjectTags after creating new Tag to + # to ensure any existing ObjectTags with the same value will + # be linked to the new Tag + object_tags = taxonomy.objecttag_set.all() + resync_object_tags(object_tags) + + return new_tag def update_tag_in_taxonomy(taxonomy: Taxonomy, tag: str, new_value: str): @@ -361,7 +370,3 @@ def delete_tags_from_taxonomy( """ taxonomy = taxonomy.cast() taxonomy.delete_tags(tags, with_subtags) - - # Resync all related ObjectTags after deleting the Tag(s) - object_tags = taxonomy.objecttag_set.all() - resync_object_tags(object_tags) From 7126e461eb83e05b131c3b094998798f6dd9c293 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Fri, 20 Oct 2023 08:11:03 +0300 Subject: [PATCH 10/13] feat: support any URL namespace for tagging REST API --- openedx_tagging/core/tagging/rest_api/v1/serializers.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index 832a371d..bceae7b7 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -120,11 +120,12 @@ def get_sub_tags_link(self, obj): """ if obj.children.count(): query_params = f"?parent_tag_id={obj.id}" + request = self.context.get("request") + url_namespace = request.resolver_match.namespace # get the namespace, usually "oel_tagging" url = ( - reverse("oel_tagging:taxonomy-tags", args=[str(obj.taxonomy_id)]) + reverse(f"{url_namespace}:taxonomy-tags", args=[str(obj.taxonomy_id)]) + query_params ) - request = self.context.get("request") return request.build_absolute_uri(url) return None From b0e4b26614e412cd2947a035b15d0adda59c475f Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Fri, 20 Oct 2023 09:52:58 +0300 Subject: [PATCH 11/13] refactor: Move validation serializer -> model method In addition to other PR requested changes: * Remove `PUT` method from docs string to avoid confusion with future changes * Replace `pk` with `id` in doc string to make it more RESTful --- openedx_tagging/core/tagging/models/base.py | 21 +++++++-- .../core/tagging/rest_api/v1/serializers.py | 44 +++---------------- .../core/tagging/rest_api/v1/views.py | 23 ++++------ 3 files changed, 32 insertions(+), 56 deletions(-) diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index ebbe5b68..a101435a 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -370,6 +370,12 @@ def add_tag( parent = None if parent_tag_value: + # Check if parent tag is valid + if not Tag.objects.filter(value__iexact=parent_tag_value).exists(): + raise ValueError("Invalid `parent_tag_value` provided") + + # Get parent tag from taxonomy, raises Tag.DoesNotExist if doesn't + # belong to taxonomy parent = self.tag_set.get(value__iexact=parent_tag_value) tag = Tag.objects.create( @@ -395,7 +401,12 @@ def update_tag(self, tag: str, new_value: str) -> Tag: "update_tag() doesn't work for system defined taxonomies. They cannot be modified." ) - # Update Tag instance with new value + # Check if tag is valid + if not Tag.objects.filter(value__iexact=tag).exists(): + raise ValueError("Invalid `tag` provided") + + # Update Tag instance with new value, raises Tag.DoesNotExist if + # tag doesn't belong to taxonomy tag_to_update = self.tag_set.get(value__iexact=tag) tag_to_update.value = new_value tag_to_update.save() @@ -419,11 +430,15 @@ def delete_tags(self, tags: List[str], with_subtags: bool = False): "delete_tags() doesn't work for system defined taxonomies. They cannot be modified." ) + # Check if tags provided are valid + if not Tag.objects.filter(value__in=tags).count() == len(tags): + raise ValueError("One or more tag in `tags` is invalid") + tags_to_delete = self.tag_set.filter(value__in=tags) if tags_to_delete.count() != len(tags): - # If they do not match that means there is a Tag ID in the provided - # list that is either invalid or does not belong to this Taxonomy + # If they do not match that means there is one or more Tag ID(s) + # provided that do not belong to this Taxonomy raise ValueError("Invalid tag id provided or tag id does not belong to taxonomy") # Check if any Tag contains subtags (children) diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index bceae7b7..c34c15da 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -197,63 +197,29 @@ def get_children_count(self, obj): class TaxonomyTagCreateBodySerializer(serializers.Serializer): # pylint: disable=abstract-method """ - Serializer of the body for the Taxonomy Tags CREATE view + Serializer of the body for the Taxonomy Tags CREATE request """ tag = serializers.CharField(required=True) - parent_tag_value = serializers.CharField( - source='parent.value', required=False - ) + parent_tag_value = serializers.CharField(required=False) external_id = serializers.CharField(required=False) - def validate_parent_tag_value(self, value): - """ - Check that the provided parent Tag exists based on the value - """ - valid = Tag.objects.filter(value__iexact=value).exists() - if not valid: - raise serializers.ValidationError("Invalid `parent_tag_value` provided") - - return value - class TaxonomyTagUpdateBodySerializer(serializers.Serializer): # pylint: disable=abstract-method """ - Serializer of the body for the Taxonomy Tags UPDATE view + Serializer of the body for the Taxonomy Tags UPDATE request """ - tag = serializers.CharField(source="value", required=True) + tag = serializers.CharField(required=True) updated_tag_value = serializers.CharField(required=True) - def validate_tag(self, value): - """ - Check that the provided Tag exists based on the value - """ - - valid = Tag.objects.filter(value__iexact=value).exists() - if not valid: - raise serializers.ValidationError("Invalid `tag` provided") - - return value - class TaxonomyTagDeleteBodySerializer(serializers.Serializer): # pylint: disable=abstract-method """ - Serializer of the body for the Taxonomy Tags DELETE view + Serializer of the body for the Taxonomy Tags DELETE request """ tags = serializers.ListField( child=serializers.CharField(), required=True ) with_subtags = serializers.BooleanField(required=False) - - def validate_tags(self, value): - """ - Check that the provided Tags exists based on the values - """ - - valid = Tag.objects.filter(value__in=value).count() == len(value) - if not valid: - raise serializers.ValidationError("One or more tag in `tags` is invalid") - - return value diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index abeb4152..6428043c 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -416,14 +416,14 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): View to list/create/update/delete tags of a taxonomy. **List Query Parameters** - * pk (required) - The pk of the taxonomy to retrieve tags. + * id (required) - The ID of the taxonomy to retrieve tags. * parent_tag_id (optional) - Id of the tag to retrieve children tags. * page (optional) - Page number (default: 1) * page_size (optional) - Number of items per page (default: 10) **List Example Requests** - GET api/tagging/v1/taxonomy/:pk/tags - Get tags of taxonomy - GET api/tagging/v1/taxonomy/:pk/tags?parent_tag_id=30 - Get children tags of tag + GET api/tagging/v1/taxonomy/:id/tags - Get tags of taxonomy + GET api/tagging/v1/taxonomy/:id/tags?parent_tag_id=30 - Get children tags of tag **List Query Returns** * 200 - Success @@ -432,7 +432,7 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): * 404 - Taxonomy not found **Create Query Parameters** - * pk (required) - The pk of the taxonomy to create a Tag for + * id (required) - The ID of the taxonomy to create a Tag for **Create Request Body** * tag (required): The value of the Tag that should be added to @@ -442,7 +442,7 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): * extenal_id (optional): The external id for the new Tag **Create Example Requests** - POST api/tagging/v1/taxonomy/:pk/tags - Create a Tag in taxonomy + POST api/tagging/v1/taxonomy/:id/tags - Create a Tag in taxonomy { "value": "New Tag", "parent_tag_value": "Parent Tag" @@ -456,19 +456,14 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): * 404 - Taxonomy not found **Update Query Parameters** - * pk (required) - The pk of the taxonomy to update a Tag in + * id (required) - The ID of the taxonomy to update a Tag in **Update Request Body** * tag (required): The value (identifier) of the Tag to be updated * updated_tag_value (required): The updated value of the Tag **Update Example Requests** - PUT api/tagging/v1/taxonomy/:pk/tags - Update a Tag in Taxonomy - { - "tag": "Tag 1", - "updated_tag_value": "Updated Tag Value" - } - PATCH api/tagging/v1/taxonomy/:pk/tags - Update a Tag in Taxonomy + PATCH api/tagging/v1/taxonomy/:id/tags - Update a Tag in Taxonomy { "tag": "Tag 1", "updated_tag_value": "Updated Tag Value" @@ -481,7 +476,7 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): * 404 - Taxonomy, Tag or Parent Tag not found **Delete Query Parameters** - * pk (required) - The pk of the taxonomy to Delete Tag(s) in + * id (required) - The ID of the taxonomy to Delete Tag(s) in **Delete Request Body** * tags (required): The values (identifiers) of Tags that should be @@ -491,7 +486,7 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): set to `True`. Defaults to `False`. **Delete Example Requests** - DELETE api/tagging/v1/taxonomy/:pk/tags - Delete Tag(s) in Taxonomy + DELETE api/tagging/v1/taxonomy/:id/tags - Delete Tag(s) in Taxonomy { "tags": ["Tag 1", "Tag 2", "Tag 3"], "with_subtags": True From d86eb91ff002fa4e711403fbd7ced0df074ad6e8 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Fri, 20 Oct 2023 10:06:38 +0300 Subject: [PATCH 12/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 d88e37a3..b602ca30 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -1,4 +1,4 @@ """ Open edX Learning ("Learning Core"). """ -__version__ = "0.2.5" +__version__ = "0.2.6" From 4b58a479d2757edc0d3f97a893bc3ce87c3d0e9a Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Sun, 22 Oct 2023 12:47:06 +0300 Subject: [PATCH 13/13] refactor: Remove redundant checks for taxonomy tags --- openedx_tagging/core/tagging/models/base.py | 12 ------------ tests/openedx_tagging/core/tagging/test_views.py | 4 ++-- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index a101435a..4ecf52c5 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -370,10 +370,6 @@ def add_tag( parent = None if parent_tag_value: - # Check if parent tag is valid - if not Tag.objects.filter(value__iexact=parent_tag_value).exists(): - raise ValueError("Invalid `parent_tag_value` provided") - # Get parent tag from taxonomy, raises Tag.DoesNotExist if doesn't # belong to taxonomy parent = self.tag_set.get(value__iexact=parent_tag_value) @@ -401,10 +397,6 @@ def update_tag(self, tag: str, new_value: str) -> Tag: "update_tag() doesn't work for system defined taxonomies. They cannot be modified." ) - # Check if tag is valid - if not Tag.objects.filter(value__iexact=tag).exists(): - raise ValueError("Invalid `tag` provided") - # Update Tag instance with new value, raises Tag.DoesNotExist if # tag doesn't belong to taxonomy tag_to_update = self.tag_set.get(value__iexact=tag) @@ -430,10 +422,6 @@ def delete_tags(self, tags: List[str], with_subtags: bool = False): "delete_tags() doesn't work for system defined taxonomies. They cannot be modified." ) - # Check if tags provided are valid - if not Tag.objects.filter(value__in=tags).count() == len(tags): - raise ValueError("One or more tag in `tags` is invalid") - tags_to_delete = self.tag_set.filter(value__in=tags) if tags_to_delete.count() != len(tags): diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 5274f7d1..b52c0e64 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -1354,7 +1354,7 @@ def test_create_tag_in_taxonomy_with_invalid_parent_tag(self): self.small_taxonomy_url, create_data, format="json" ) - assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.status_code == status.HTTP_404_NOT_FOUND def test_create_tag_in_taxonomy_with_parent_tag_in_other_taxonomy(self): self.client.force_authenticate(user=self.staff) @@ -1541,7 +1541,7 @@ def test_update_tag_in_taxonomy_with_invalid_tag(self): self.small_taxonomy_url, update_data, format="json" ) - assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.status_code == status.HTTP_404_NOT_FOUND def test_update_tag_in_taxonomy_with_tag_in_other_taxonomy(self): self.client.force_authenticate(user=self.staff)