From 8e550d77842fef780658c637fafd56725a699352 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Tue, 17 Oct 2023 17:06:04 +0300 Subject: [PATCH] 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, 114 insertions(+), 82 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..683888dd 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -1,6 +1,7 @@ """ API Serializers for taxonomies """ +from functools import reduce from rest_framework import serializers from rest_framework.reverse import reverse @@ -201,21 +202,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 +243,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(