From e3ef5a5d179fe4534728599d84d1d6ee888bb242 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Fri, 26 Jan 2024 11:06:15 -0500 Subject: [PATCH 1/8] feat: Add export_id field on Taxonomy --- openedx_tagging/core/tagging/api.py | 4 + .../core/tagging/import_export/api.py | 2 +- .../migrations/0015_taxonomy_export_id.py | 37 ++++++++++ openedx_tagging/core/tagging/models/base.py | 23 ++++++ .../core/tagging/rest_api/v1/serializers.py | 12 +++ .../core/tagging/rest_api/v1/views.py | 21 +++++- .../core/fixtures/tagging.yaml | 5 +- .../core/tagging/import_export/test_api.py | 2 +- .../openedx_tagging/core/tagging/test_api.py | 21 ++++-- .../core/tagging/test_models.py | 51 ++++++++++++- .../tagging/test_system_defined_models.py | 4 + .../core/tagging/test_views.py | 74 +++++++++++++------ 12 files changed, 221 insertions(+), 35 deletions(-) create mode 100644 openedx_tagging/core/tagging/migrations/0015_taxonomy_export_id.py diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index 9cde39a47..d2db72049 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -29,6 +29,7 @@ def create_taxonomy( name: str, + export_id: str, description: str | None = None, enabled=True, allow_multiple=True, @@ -44,9 +45,12 @@ def create_taxonomy( enabled=enabled, allow_multiple=allow_multiple, allow_free_text=allow_free_text, + export_id=export_id, ) if taxonomy_class: taxonomy.taxonomy_class = taxonomy_class + + taxonomy.full_clean() taxonomy.save() return taxonomy.cast() diff --git a/openedx_tagging/core/tagging/import_export/api.py b/openedx_tagging/core/tagging/import_export/api.py index 34c496878..94d628c59 100644 --- a/openedx_tagging/core/tagging/import_export/api.py +++ b/openedx_tagging/core/tagging/import_export/api.py @@ -121,7 +121,7 @@ def import_tags( task.end_success() return True, task, tag_import_plan - except Exception as exception: # pylint: disable=broad-exception-caught + except Exception as exception: # Log any exception task.log_exception(exception) return False, task, None diff --git a/openedx_tagging/core/tagging/migrations/0015_taxonomy_export_id.py b/openedx_tagging/core/tagging/migrations/0015_taxonomy_export_id.py new file mode 100644 index 000000000..f30438e64 --- /dev/null +++ b/openedx_tagging/core/tagging/migrations/0015_taxonomy_export_id.py @@ -0,0 +1,37 @@ +# Generated by Django 3.2.22 on 2024-01-25 14:20 + +from django.db import migrations, models + + +def migrate_export_id(apps, schema_editor): + Taxonomy = apps.get_model("oel_tagging", "Taxonomy") + for taxonomy in Taxonomy.objects.all(): + # Adds the id of the taxonomy to avoid duplicates + taxonomy.export_id = f"{taxonomy.id}_{taxonomy.name.lower().replace(' ', '.')}" + taxonomy.save(update_fields=["export_id"]) + +def reverse(app, schema_editor): + pass + +class Migration(migrations.Migration): + + dependencies = [ + ('oel_tagging', '0014_minor_fixes'), + ] + + operations = [ + # Create the field allowing null + migrations.AddField( + model_name='taxonomy', + name='export_id', + field=models.CharField(help_text='External ID that is used on import/export', max_length=255, null=True, unique=True), + ), + # Fill the field for created taxonomies + migrations.RunPython(migrate_export_id, reverse), + # Alter the field to not allowing null + migrations.AlterField( + model_name='taxonomy', + name='export_id', + field=models.CharField(help_text='External ID that is used on import/export', max_length=255, null=False, unique=True), + ), + ] diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index 13beeb0a2..c9b4439ec 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -4,6 +4,7 @@ from __future__ import annotations import logging +import re from typing import List from django.core.exceptions import ValidationError @@ -228,6 +229,19 @@ class Taxonomy(models.Model): "Indicates whether this taxonomy should be visible to object authors." ), ) + # External ID that should only be used on import/export. + # NOT use for any other purposes, you can use the numeric ID of the model instead; + # this id is editable. + export_id = models.CharField( + null=False, + blank=False, + max_length=255, + help_text=_( + "User-facing ID that is used on import/export." + " Should only contain alphanumeric characters or '_' '-' '.'" + ), + unique=True, + ) _taxonomy_class = models.CharField( null=True, max_length=255, @@ -300,6 +314,14 @@ def system_defined(self) -> bool: """ return False + def clean(self): + super().clean() + + if not re.match(r'^[\w\-.]+$', self.export_id): + raise ValidationError( + "The export_id should only contain alphanumeric characters or '_' '-' '.'" + ) + def cast(self): """ Returns the current Taxonomy instance cast into its taxonomy_class. @@ -336,6 +358,7 @@ def copy(self, taxonomy: Taxonomy) -> Taxonomy: self.allow_multiple = taxonomy.allow_multiple self.allow_free_text = taxonomy.allow_free_text self.visible_to_authors = taxonomy.visible_to_authors + self.export_id = taxonomy.export_id self._taxonomy_class = taxonomy._taxonomy_class # pylint: disable=protected-access return self diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index a3af0bbbb..ea71078bb 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -88,8 +88,19 @@ class Meta: "can_change_taxonomy", "can_delete_taxonomy", "can_tag_object", + "export_id", ] + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + if self.context['request'].method in ('PUT', 'PATCH'): + # Makes export_id field optional during update + self.fields['export_id'].required = False + elif self.context['request'].method == 'POST': + # Makes the export_id field mandatory during creation + self.fields['export_id'].required = True + def to_representation(self, instance): """ Cast the taxonomy before serialize @@ -332,6 +343,7 @@ class TaxonomyImportNewBodySerializer(TaxonomyImportBodySerializer): # pylint: """ taxonomy_name = serializers.CharField(required=True) taxonomy_description = serializers.CharField(default="") + taxonomy_export_id = serializers.CharField(required=True) class TagImportTaskSerializer(serializers.ModelSerializer): diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index 3abf4f0d5..cdff526fd 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -3,6 +3,7 @@ """ from __future__ import annotations +from django.core import exceptions from django.db import models from django.http import Http404, HttpResponse from rest_framework import mixins, status @@ -57,6 +58,11 @@ class TaxonomyView(ModelViewSet): """ View to list, create, retrieve, update, delete, export or import Taxonomies. + TODO: We need to add a perform_udate and call the api update function when is created. + This is because it is necessary to call the model validations. (`full_clean()`). + Currently those validations are not run, which means we could update `export_id` with any value + through this api. + **List Query Parameters** * enabled (optional) - Filter by enabled status. Valid values: true, false, 1, 0, "true", "false", "1" @@ -87,6 +93,8 @@ class TaxonomyView(ModelViewSet): **Create Parameters** * name (required): User-facing label used when applying tags from this taxonomy to Open edX objects. + * export_id (required): User-facing ID that is used on import/export. + Should only contain alphanumeric characters or '_' '-' '.'. * description (optional): Provides extra information for the user when applying tags from this taxonomy to an object. * enabled (optional): Only enabled taxonomies will be shown to authors @@ -101,6 +109,7 @@ class TaxonomyView(ModelViewSet): POST api/tagging/v1/taxonomy - Create a taxonomy { "name": "Taxonomy Name", + "export_id": "taxonomy_export_id", "description": "This is a description", "enabled": True, "allow_multiple": True, @@ -117,6 +126,8 @@ class TaxonomyView(ModelViewSet): **Update Request Body** * name (optional): User-facing label used when applying tags from this taxonomy to Open edX objects. + * export_id (optional): User-facing ID that is used on import/export. + Should only contain alphanumeric characters or '_' '-' '.'. * description (optional): Provides extra information for the user when applying tags from this taxonomy to an object. * enabled (optional): Only enabled taxonomies will be shown to authors. @@ -129,6 +140,7 @@ class TaxonomyView(ModelViewSet): PUT api/tagging/v1/taxonomy/:pk - Update a taxonomy { "name": "Taxonomy New Name", + "export_id": "taxonomy_new_name", "description": "This is a new description", "enabled": False, "allow_multiple": False, @@ -174,6 +186,7 @@ class TaxonomyView(ModelViewSet): POST /tagging/rest_api/v1/taxonomy/import/ { "taxonomy_name": "Taxonomy Name", + "taxonomy_export_id": "this_is_the_export_id", "taxonomy_description": "This is a description", "file": , } @@ -245,7 +258,10 @@ def perform_create(self, serializer) -> None: """ Create a new taxonomy. """ - serializer.instance = create_taxonomy(**serializer.validated_data) + try: + serializer.instance = create_taxonomy(**serializer.validated_data) + except exceptions.ValidationError as e: + raise ValidationError() from e @action(detail=True, methods=["get"]) def export(self, request, **_kwargs) -> HttpResponse: @@ -286,11 +302,12 @@ def create_import(self, request: Request, **_kwargs) -> Response: body.is_valid(raise_exception=True) taxonomy_name = body.validated_data["taxonomy_name"] + taxonomy_export_id = body.validated_data["taxonomy_export_id"] taxonomy_description = body.validated_data["taxonomy_description"] file = body.validated_data["file"].file parser_format = body.validated_data["parser_format"] - taxonomy = create_taxonomy(taxonomy_name, taxonomy_description) + taxonomy = create_taxonomy(taxonomy_name, taxonomy_export_id, taxonomy_description) try: import_success, task, _plan = import_tags(taxonomy, file, parser_format) diff --git a/tests/openedx_tagging/core/fixtures/tagging.yaml b/tests/openedx_tagging/core/fixtures/tagging.yaml index 164b93990..cd0204f3b 100644 --- a/tests/openedx_tagging/core/fixtures/tagging.yaml +++ b/tests/openedx_tagging/core/fixtures/tagging.yaml @@ -230,6 +230,7 @@ enabled: true allow_multiple: false allow_free_text: false + export_id: life_on_earth - model: oel_tagging.taxonomy pk: 3 fields: @@ -238,6 +239,7 @@ enabled: true allow_multiple: false allow_free_text: false + export_id: user_authors _taxonomy_class: openedx_tagging.core.tagging.models.system_defined.UserSystemDefinedTaxonomy - model: oel_tagging.taxonomy pk: 4 @@ -247,6 +249,7 @@ enabled: true allow_multiple: false allow_free_text: false + export_id: system_defined_taxonomy _taxonomy_class: openedx_tagging.core.tagging.models.system_defined.SystemDefinedTaxonomy - model: oel_tagging.taxonomy pk: 5 @@ -256,4 +259,4 @@ enabled: true allow_multiple: false allow_free_text: false - + export_id: import_taxonomy_test diff --git a/tests/openedx_tagging/core/tagging/import_export/test_api.py b/tests/openedx_tagging/core/tagging/import_export/test_api.py index d23eb6dd1..441587c45 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_api.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_api.py @@ -197,7 +197,7 @@ def test_import_with_export_output(self) -> None: parser_format, ) file = BytesIO(output.encode()) - new_taxonomy = Taxonomy(name="New taxonomy") + new_taxonomy = Taxonomy(name="New taxonomy", export_id=f"new_taxonomy_{parser_format}") new_taxonomy.save() result, _task, _plan = import_export_api.import_tags( new_taxonomy, diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 5befef5c9..fa3aeb4f5 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -56,6 +56,7 @@ def test_create_taxonomy(self) -> None: # Note: we must specify '-> None' to op "enabled": False, "allow_multiple": True, "allow_free_text": True, + "export_id": "difficulty", } taxonomy = tagging_api.create_taxonomy(**params) for param, value in params.items(): @@ -67,6 +68,7 @@ def test_bad_taxonomy_class(self) -> None: with self.assertRaises(ValueError) as exc: tagging_api.create_taxonomy( name="Bad class", + export_id="bad_class", taxonomy_class=str, # type: ignore[arg-type] ) assert " must be a subclass of Taxonomy" in str(exc.exception) @@ -78,8 +80,8 @@ def test_get_taxonomy(self) -> None: assert no_tax is None def test_get_taxonomies(self) -> None: - tax1 = tagging_api.create_taxonomy("Enabled") - tax2 = tagging_api.create_taxonomy("Disabled", enabled=False) + tax1 = tagging_api.create_taxonomy("Enabled", "enabled") + tax2 = tagging_api.create_taxonomy("Disabled", "disabled", enabled=False) tax3 = Taxonomy.objects.get(name="Import Taxonomy Test") with self.assertNumQueries(1): enabled = list(tagging_api.get_taxonomies()) @@ -242,7 +244,11 @@ def test_get_children_tags_invalid_taxonomy(self) -> None: """ Calling get_children_tags on free text taxonomies gives an error. """ - free_text_taxonomy = Taxonomy.objects.create(allow_free_text=True, name="FreeText") + free_text_taxonomy = Taxonomy.objects.create( + allow_free_text=True, + name="FreeText", + export_id="free_text" + ) tagging_api.tag_object(object_id="obj1", taxonomy=free_text_taxonomy, tags=["some_tag"]) with self.assertRaises(ValueError) as exc: tagging_api.get_children_tags(free_text_taxonomy, "some_tag") @@ -257,7 +263,12 @@ def test_get_children_tags_no_children(self) -> None: def test_resync_object_tags(self) -> None: self.taxonomy.allow_multiple = True self.taxonomy.save() - open_taxonomy = Taxonomy.objects.create(name="Freetext Life", allow_free_text=True, allow_multiple=True) + open_taxonomy = Taxonomy.objects.create( + name="Freetext Life", + allow_free_text=True, + allow_multiple=True, + export_id='freetext_life', + ) object_id = "obj1" # Create some tags: @@ -571,7 +582,7 @@ def test_get_object_tags_deleted_disabled(self) -> None: obj_id = "object_id1" self.taxonomy.allow_multiple = True self.taxonomy.save() - disabled_taxonomy = tagging_api.create_taxonomy("Disabled Taxonomy", allow_free_text=True) + disabled_taxonomy = tagging_api.create_taxonomy("Disabled Taxonomy", "disabled_taxonomy", allow_free_text=True) tagging_api.tag_object(object_id=obj_id, taxonomy=self.taxonomy, tags=["DPANN", "Chordata"]) tagging_api.tag_object(object_id=obj_id, taxonomy=self.language_taxonomy, tags=["English"]) tagging_api.tag_object(object_id=obj_id, taxonomy=self.free_text_taxonomy, tags=["has a notochord"]) diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index c730b16bc..abfd1a528 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -80,7 +80,7 @@ def create_sort_test_taxonomy(self) -> Taxonomy: Helper method to create a taxonomy that's difficult to sort correctly in tree order. """ # pylint: disable=unused-variable - taxonomy = api.create_taxonomy("Sort Test") + taxonomy = api.create_taxonomy("Sort Test", "sort_test") root1 = Tag.objects.create(taxonomy=taxonomy, value="1") child1_1 = Tag.objects.create(taxonomy=taxonomy, value="11", parent=root1) @@ -114,7 +114,8 @@ def create_100_taxonomies(self): taxonomy = Taxonomy.objects.create( name=f"ZZ Dummy Taxonomy {i:03}", allow_free_text=True, - allow_multiple=True + allow_multiple=True, + export_id=f"zz_dummy_taxonomy_{i:03}" ) ObjectTag.objects.create( object_id="limit_tag_count", @@ -203,7 +204,7 @@ def test_taxonomy_cast(self): def test_taxonomy_cast_import_error(self): taxonomy = Taxonomy.objects.create( - name="Invalid cast", _taxonomy_class="not.a.class" + name="Invalid cast", export_id='invalid_cast', _taxonomy_class="not.a.class" ) # Error is logged, but ignored. cast_taxonomy = taxonomy.cast() @@ -268,6 +269,50 @@ def test_no_tab(self): with pytest.raises(ValidationError): api.add_tag_to_taxonomy(self.taxonomy, "first\tsecond") + @ddt.data( + ("test"), + ("lightcast"), + ("lightcast-skills"), + ("io.lightcast.open-skills"), + ("-3_languages"), + ("LIGHTCAST_V17"), + ("liGhtCaST"), + ("日本"), + ("Québec"), + ("123456789"), + ) + def test_export_id_format_valid(self, export_id): + self.taxonomy.export_id = export_id + self.taxonomy.full_clean() + + @ddt.data( + ("LightCast Skills"), + ("One,Two,Three"), + (" "), + ("Foo:Bar"), + ("X;Y;Z"), + ('"quotes"'), + (" test"), + ) + def test_export_id_format_invalid(self, export_id): + self.taxonomy.export_id = export_id + with pytest.raises(ValidationError): + self.taxonomy.full_clean() + + def test_unique_export_id(self): + # Valid + self.taxonomy.export_id = 'test_1' + self.free_text_taxonomy.export_id = 'test_2' + self.taxonomy.save() + self.free_text_taxonomy.save() + + # Invalid + self.taxonomy.export_id = 'test_1' + self.free_text_taxonomy.export_id = 'test_1' + self.taxonomy.save() + with pytest.raises(IntegrityError): + self.free_text_taxonomy.save() + @ddt.ddt class TestFilteredTagsClosedTaxonomy(TestTagTaxonomyMixin, TestCase): diff --git a/tests/openedx_tagging/core/tagging/test_system_defined_models.py b/tests/openedx_tagging/core/tagging/test_system_defined_models.py index 3c0cb6763..e75754ac1 100644 --- a/tests/openedx_tagging/core/tagging/test_system_defined_models.py +++ b/tests/openedx_tagging/core/tagging/test_system_defined_models.py @@ -93,12 +93,14 @@ def setUpClass(cls): taxonomy_class=LPTaxonomyTest, name="LearningPackage Taxonomy", allow_multiple=True, + export_id="learning_package_taxonomy", ) # Also create an "Author" taxonomy that can tag any object using user IDs/usernames: cls.author_taxonomy = UserSystemDefinedTaxonomy.objects.create( taxonomy_class=UserSystemDefinedTaxonomy, name="Authors", allow_multiple=True, + export_id="authors", ) def test_lp_taxonomy_validation(self): @@ -169,6 +171,7 @@ def test_case_insensitive_values(self): taxonomy_class=CaseInsensitiveTitleLPTaxonomy, name="LearningPackage Title Taxonomy", allow_multiple=True, + export_id="learning_package_title_taxonomy", ) api.tag_object(taxonomy, ["LEARNING PACKAGE 1"], object1_id) api.tag_object(taxonomy, ["Learning Package 1", "LEARNING PACKAGE 2"], object2_id) @@ -184,6 +187,7 @@ def test_multiple_taxonomies(self): taxonomy_class=UserSystemDefinedTaxonomy, name="Reviewer", allow_multiple=True, + export_id="reviewer", ) pr_1_id, pr_2_id = "pull_request_1", "pull_request_2" diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index c2ccd320a..4d95ccb7b 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -56,6 +56,7 @@ def check_taxonomy( can_change_taxonomy=None, can_delete_taxonomy=None, can_tag_object=None, + export_id=None, ): """ Check taxonomy data @@ -71,6 +72,16 @@ def check_taxonomy( assert data["can_change_taxonomy"] == can_change_taxonomy assert data["can_delete_taxonomy"] == can_delete_taxonomy assert data["can_tag_object"] == can_tag_object + assert data["export_id"] == export_id + + +def create_taxonomy(name, description=None, enabled=True): + return api.create_taxonomy( + name, + export_id=name.lower().replace(" ", "_"), + description=description, + enabled=enabled, + ) class TestTaxonomyViewMixin(APITestCase): @@ -114,9 +125,9 @@ class TestTaxonomyViewSet(TestTaxonomyViewMixin): ) @ddt.unpack def test_list_taxonomy_queryparams(self, enabled, expected_status: int, expected_count: int | None): - api.create_taxonomy(name="Taxonomy enabled 1", enabled=True) - api.create_taxonomy(name="Taxonomy enabled 2", enabled=True) - api.create_taxonomy(name="Taxonomy disabled", enabled=False) + create_taxonomy(name="Taxonomy enabled 1", enabled=True) + create_taxonomy(name="Taxonomy enabled 2", enabled=True) + create_taxonomy(name="Taxonomy disabled", enabled=False) url = TAXONOMY_LIST_URL @@ -139,7 +150,7 @@ def test_list_taxonomy_queryparams(self, enabled, expected_status: int, expected ) @ddt.unpack def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_count: int, is_admin=False): - taxonomy = api.create_taxonomy(name="Taxonomy enabled 1", enabled=True) + taxonomy = create_taxonomy(name="Taxonomy enabled 1", enabled=True) for i in range(tags_count): tag = Tag( taxonomy=taxonomy, @@ -173,6 +184,7 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c "can_change_taxonomy": False, "can_delete_taxonomy": False, "can_tag_object": False, + "export_id": "-1_languages", }, { "id": taxonomy.id, @@ -190,17 +202,18 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c # can_tag_object is False because we default to not allowing users to tag arbitrary objects. # But specific uses of this code (like content_tagging) will override this perm for their use cases. "can_tag_object": False, + "export_id": "taxonomy_enabled_1", }, ] assert response.data.get("can_add_taxonomy") == is_admin def test_list_taxonomy_pagination(self) -> None: url = TAXONOMY_LIST_URL - api.create_taxonomy(name="T1", enabled=True) - api.create_taxonomy(name="T2", enabled=True) - api.create_taxonomy(name="T3", enabled=False) - api.create_taxonomy(name="T4", enabled=False) - api.create_taxonomy(name="T5", enabled=False) + create_taxonomy(name="T1", enabled=True) + create_taxonomy(name="T2", enabled=True) + create_taxonomy(name="T3", enabled=False) + create_taxonomy(name="T4", enabled=False) + create_taxonomy(name="T5", enabled=False) self.client.force_authenticate(user=self.staff) @@ -247,8 +260,8 @@ def test_list_taxonomy_query_count(self): """ Test how many queries are used when retrieving taxonomies and permissions """ - api.create_taxonomy(name="T1", enabled=True) - api.create_taxonomy(name="T2", enabled=True) + create_taxonomy(name="T1", enabled=True) + create_taxonomy(name="T2", enabled=True) url = TAXONOMY_LIST_URL @@ -294,6 +307,7 @@ def test_language_taxonomy(self): can_change_taxonomy=False, can_delete_taxonomy=False, can_tag_object=False, + export_id='-1_languages', ) @ddt.data( @@ -308,7 +322,11 @@ def test_language_taxonomy(self): def test_detail_taxonomy( self, user_attr: str | None, taxonomy_data: dict[str, bool], expected_status: int, is_admin=False, ): - create_data = {"name": "taxonomy detail test", **taxonomy_data} + create_data = { + "name": "taxonomy detail test", + "export_id": "taxonomy_detail_test", + **taxonomy_data + } taxonomy = api.create_taxonomy(**create_data) # type: ignore[arg-type] url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) @@ -361,6 +379,7 @@ def test_create_taxonomy(self, user_attr: str | None, expected_status: int): "description": "This is a description", "enabled": False, "allow_multiple": True, + "export_id": 'taxonomy_data_2', } if user_attr: @@ -383,7 +402,9 @@ def test_create_taxonomy(self, user_attr: str | None, expected_status: int): @ddt.data( {}, - {"name": "Error taxonomy 3", "enabled": "Invalid value"}, + {"name": "Error taxonomy"}, # Without export_id + {"name": "Error taxonomy 1", "export_id": "Invalid value"}, # Invalid export_id + {"name": "Error taxonomy 3", "export_id": "test", "enabled": "Invalid value"}, ) def test_create_taxonomy_error(self, create_data: dict[str, str]): url = TAXONOMY_LIST_URL @@ -392,7 +413,7 @@ def test_create_taxonomy_error(self, create_data: dict[str, str]): response = self.client.post(url, create_data, format="json") assert response.status_code == status.HTTP_400_BAD_REQUEST - @ddt.data({"name": "System defined taxonomy", "system_defined": True}) + @ddt.data({"name": "System defined taxonomy", "export_id": "sd", "system_defined": True}) def test_create_taxonomy_system_defined(self, create_data): """ Cannont create a taxonomy with system_defined=true @@ -411,7 +432,7 @@ def test_create_taxonomy_system_defined(self, create_data): ) @ddt.unpack def test_update_taxonomy(self, user_attr, expected_status): - taxonomy = api.create_taxonomy( + taxonomy = create_taxonomy( name="test update taxonomy", description="taxonomy description", enabled=True, @@ -439,6 +460,7 @@ def test_update_taxonomy(self, user_attr, expected_status): "can_change_taxonomy": True, "can_delete_taxonomy": True, "can_tag_object": False, + "export_id": "test_update_taxonomy", }, ) @@ -453,6 +475,7 @@ def test_update_taxonomy_system_defined(self, system_defined, expected_status): """ taxonomy = api.create_taxonomy( name="test system taxonomy", + export_id="test_system_taxonomy", taxonomy_class=SystemDefinedTaxonomy if system_defined else None, ) url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) @@ -475,7 +498,7 @@ def test_update_taxonomy_404(self): ) @ddt.unpack def test_patch_taxonomy(self, user_attr, expected_status): - taxonomy = api.create_taxonomy(name="test patch taxonomy", enabled=False) + taxonomy = create_taxonomy(name="test patch taxonomy", enabled=False) url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) @@ -498,6 +521,7 @@ def test_patch_taxonomy(self, user_attr, expected_status): "can_change_taxonomy": True, "can_delete_taxonomy": True, "can_tag_object": False, + "export_id": 'test_patch_taxonomy', }, ) @@ -512,6 +536,7 @@ def test_patch_taxonomy_system_defined(self, system_defined, expected_status): """ taxonomy = api.create_taxonomy( name="test system taxonomy", + export_id="test_system_taxonomy", taxonomy_class=SystemDefinedTaxonomy if system_defined else None, ) url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) @@ -534,7 +559,7 @@ def test_patch_taxonomy_404(self): ) @ddt.unpack def test_delete_taxonomy(self, user_attr, expected_status): - taxonomy = api.create_taxonomy(name="test delete taxonomy") + taxonomy = create_taxonomy(name="test delete taxonomy") url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) @@ -566,7 +591,7 @@ def test_export_taxonomy(self, output_format, content_type): """ Tests if a user can export a taxonomy """ - taxonomy = api.create_taxonomy(name="T1") + taxonomy = create_taxonomy(name="T1") for i in range(20): # Valid ObjectTags Tag.objects.create(taxonomy=taxonomy, value=f"Tag {i}").save() @@ -593,7 +618,7 @@ def test_export_taxonomy_download(self, output_format, content_type): """ Tests if a user can export a taxonomy with download option """ - taxonomy = api.create_taxonomy(name="T1") + taxonomy = create_taxonomy(name="T1") for i in range(20): api.add_tag_to_taxonomy(taxonomy=taxonomy, tag=f"Tag {i}") @@ -615,7 +640,7 @@ def test_export_taxonomy_invalid_param_output_format(self): """ Tests if a user can export a taxonomy using an invalid output_format param """ - taxonomy = api.create_taxonomy(name="T1") + taxonomy = create_taxonomy(name="T1") url = TAXONOMY_EXPORT_URL.format(pk=taxonomy.pk) @@ -627,7 +652,7 @@ def test_export_taxonomy_invalid_param_download(self): """ Tests if a user can export a taxonomy using an invalid output_format param """ - taxonomy = api.create_taxonomy(name="T1") + taxonomy = create_taxonomy(name="T1") url = TAXONOMY_EXPORT_URL.format(pk=taxonomy.pk) @@ -640,7 +665,7 @@ def test_export_taxonomy_unauthorized(self): Tests if a user can export a taxonomy that he doesn't have authorization """ # Only staff can view a disabled taxonomy - taxonomy = api.create_taxonomy(name="T1", enabled=False) + taxonomy = create_taxonomy(name="T1", enabled=False) url = TAXONOMY_EXPORT_URL.format(pk=taxonomy.pk) @@ -2343,6 +2368,7 @@ def test_import(self, file_format: str) -> None: { "taxonomy_name": "Imported Taxonomy name", "taxonomy_description": "Imported Taxonomy description", + "taxonomy_export_id": "imported_taxonomy_export_id", "file": file, }, format="multipart" @@ -2373,6 +2399,7 @@ def test_import_no_file(self) -> None: { "taxonomy_name": "Imported Taxonomy name", "taxonomy_description": "Imported Taxonomy description", + "taxonomy_export_id": "imported_taxonomy_description", }, format="multipart" ) @@ -2419,6 +2446,7 @@ def test_import_invalid_format(self) -> None: { "taxonomy_name": "Imported Taxonomy name", "taxonomy_description": "Imported Taxonomy description", + "taxonomy_export_id": "imported_taxonomy_description", "file": file, }, format="multipart" @@ -2445,6 +2473,7 @@ def test_import_invalid_content(self, file_format) -> None: { "taxonomy_name": "Imported Taxonomy name", "taxonomy_description": "Imported Taxonomy description", + "taxonomy_export_id": "imported_taxonomy_export_id", "file": file, }, format="multipart" @@ -2505,6 +2534,7 @@ def setUp(self): self.taxonomy = Taxonomy.objects.create( name="Test import taxonomy", + export_id="test_import_taxonomy", ) tag_1 = Tag.objects.create( taxonomy=self.taxonomy, From e0062ba8ab5bad18f144bf81c2126762c355d6c9 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Tue, 30 Jan 2024 08:54:52 -0500 Subject: [PATCH 2/8] 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 13243d24e..0dc1d5053 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -1,4 +1,4 @@ """ Open edX Learning ("Learning Core"). """ -__version__ = "0.4.4" +__version__ = "0.5.0" From 2bece8f1966965a667ea1bb5e35144f26a478039 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Tue, 30 Jan 2024 09:38:42 -0500 Subject: [PATCH 3/8] chore: Update migration --- .../core/tagging/migrations/0015_taxonomy_export_id.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openedx_tagging/core/tagging/migrations/0015_taxonomy_export_id.py b/openedx_tagging/core/tagging/migrations/0015_taxonomy_export_id.py index f30438e64..39c1cd795 100644 --- a/openedx_tagging/core/tagging/migrations/0015_taxonomy_export_id.py +++ b/openedx_tagging/core/tagging/migrations/0015_taxonomy_export_id.py @@ -24,7 +24,7 @@ class Migration(migrations.Migration): migrations.AddField( model_name='taxonomy', name='export_id', - field=models.CharField(help_text='External ID that is used on import/export', max_length=255, null=True, unique=True), + field=models.CharField(help_text="User-facing ID that is used on import/export. Should only contain alphanumeric characters or '_' '-' '.'", max_length=255, null=True, unique=True), ), # Fill the field for created taxonomies migrations.RunPython(migrate_export_id, reverse), @@ -32,6 +32,6 @@ class Migration(migrations.Migration): migrations.AlterField( model_name='taxonomy', name='export_id', - field=models.CharField(help_text='External ID that is used on import/export', max_length=255, null=False, unique=True), + field=models.CharField(help_text="User-facing ID that is used on import/export. Should only contain alphanumeric characters or '_' '-' '.'", max_length=255, null=False, unique=True), ), ] From db64b9fc41bd75c9a38a2e9d3c0a4b1338db82f9 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Tue, 30 Jan 2024 10:52:15 -0500 Subject: [PATCH 4/8] style: Replace '.' to '_' in migration --- .../core/tagging/migrations/0015_taxonomy_export_id.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_tagging/core/tagging/migrations/0015_taxonomy_export_id.py b/openedx_tagging/core/tagging/migrations/0015_taxonomy_export_id.py index 39c1cd795..2a9bb2b56 100644 --- a/openedx_tagging/core/tagging/migrations/0015_taxonomy_export_id.py +++ b/openedx_tagging/core/tagging/migrations/0015_taxonomy_export_id.py @@ -7,7 +7,7 @@ def migrate_export_id(apps, schema_editor): Taxonomy = apps.get_model("oel_tagging", "Taxonomy") for taxonomy in Taxonomy.objects.all(): # Adds the id of the taxonomy to avoid duplicates - taxonomy.export_id = f"{taxonomy.id}_{taxonomy.name.lower().replace(' ', '.')}" + taxonomy.export_id = f"{taxonomy.id}_{taxonomy.name.lower().replace(' ', '_')}" taxonomy.save(update_fields=["export_id"]) def reverse(app, schema_editor): From b341e9b7f75cdde070400354399b77ac8f09b799 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Wed, 31 Jan 2024 11:03:05 -0500 Subject: [PATCH 5/8] feat: Adding slugify in migration --- .../core/tagging/migrations/0015_taxonomy_export_id.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/openedx_tagging/core/tagging/migrations/0015_taxonomy_export_id.py b/openedx_tagging/core/tagging/migrations/0015_taxonomy_export_id.py index 2a9bb2b56..60115b46c 100644 --- a/openedx_tagging/core/tagging/migrations/0015_taxonomy_export_id.py +++ b/openedx_tagging/core/tagging/migrations/0015_taxonomy_export_id.py @@ -1,13 +1,14 @@ # Generated by Django 3.2.22 on 2024-01-25 14:20 from django.db import migrations, models +from django.utils.text import slugify def migrate_export_id(apps, schema_editor): Taxonomy = apps.get_model("oel_tagging", "Taxonomy") for taxonomy in Taxonomy.objects.all(): # Adds the id of the taxonomy to avoid duplicates - taxonomy.export_id = f"{taxonomy.id}_{taxonomy.name.lower().replace(' ', '_')}" + taxonomy.export_id = f"{taxonomy.id}_{slugify(taxonomy.name, allow_unicode=True)}" taxonomy.save(update_fields=["export_id"]) def reverse(app, schema_editor): From 4b695682bfbbcfa263b697db87f767c7807d304d Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Wed, 31 Jan 2024 11:57:55 -0500 Subject: [PATCH 6/8] refactor: Auto generate export_id on create_taxonomy API if the param is empty --- openedx_tagging/core/tagging/api.py | 6 +- .../migrations/0015_taxonomy_export_id.py | 2 +- .../core/tagging/rest_api/v1/serializers.py | 11 +-- .../core/tagging/rest_api/v1/views.py | 2 +- .../openedx_tagging/core/tagging/test_api.py | 16 +++- .../core/tagging/test_models.py | 2 +- .../core/tagging/test_views.py | 84 +++++++++++-------- 7 files changed, 70 insertions(+), 53 deletions(-) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index d2db72049..c416f6f09 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -17,6 +17,7 @@ from django.db import models, transaction from django.db.models import F, QuerySet, Value from django.db.models.functions import Coalesce, Concat, Lower +from django.utils.text import slugify from django.utils.translation import gettext as _ from .data import TagDataQuerySet @@ -29,16 +30,19 @@ def create_taxonomy( name: str, - export_id: str, description: str | None = None, enabled=True, allow_multiple=True, allow_free_text=False, taxonomy_class: type[Taxonomy] | None = None, + export_id: str | None = None, ) -> Taxonomy: """ Creates, saves, and returns a new Taxonomy with the given attributes. """ + if not export_id: + export_id = f"{Taxonomy.objects.count() + 1}-{slugify(name, allow_unicode=True)}" + taxonomy = Taxonomy( name=name, description=description or "", diff --git a/openedx_tagging/core/tagging/migrations/0015_taxonomy_export_id.py b/openedx_tagging/core/tagging/migrations/0015_taxonomy_export_id.py index 60115b46c..cd654dd0f 100644 --- a/openedx_tagging/core/tagging/migrations/0015_taxonomy_export_id.py +++ b/openedx_tagging/core/tagging/migrations/0015_taxonomy_export_id.py @@ -8,7 +8,7 @@ def migrate_export_id(apps, schema_editor): Taxonomy = apps.get_model("oel_tagging", "Taxonomy") for taxonomy in Taxonomy.objects.all(): # Adds the id of the taxonomy to avoid duplicates - taxonomy.export_id = f"{taxonomy.id}_{slugify(taxonomy.name, allow_unicode=True)}" + taxonomy.export_id = f"{taxonomy.id}-{slugify(taxonomy.name, allow_unicode=True)}" taxonomy.save(update_fields=["export_id"]) def reverse(app, schema_editor): diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index ea71078bb..10e3730e2 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -72,6 +72,7 @@ class TaxonomySerializer(UserPermissionsSerializerMixin, serializers.ModelSerial can_change_taxonomy = serializers.SerializerMethodField(method_name='get_can_change') can_delete_taxonomy = serializers.SerializerMethodField(method_name='get_can_delete') can_tag_object = serializers.SerializerMethodField() + export_id = serializers.CharField(required=False) class Meta: model = Taxonomy @@ -91,16 +92,6 @@ class Meta: "export_id", ] - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - - if self.context['request'].method in ('PUT', 'PATCH'): - # Makes export_id field optional during update - self.fields['export_id'].required = False - elif self.context['request'].method == 'POST': - # Makes the export_id field mandatory during creation - self.fields['export_id'].required = True - def to_representation(self, instance): """ Cast the taxonomy before serialize diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index cdff526fd..acb45fa0d 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -307,7 +307,7 @@ def create_import(self, request: Request, **_kwargs) -> Response: file = body.validated_data["file"].file parser_format = body.validated_data["parser_format"] - taxonomy = create_taxonomy(taxonomy_name, taxonomy_export_id, taxonomy_description) + taxonomy = create_taxonomy(taxonomy_name, taxonomy_description, export_id=taxonomy_export_id) try: import_success, task, _plan = import_tags(taxonomy, file, parser_format) diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index fa3aeb4f5..bad649da0 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -64,6 +64,16 @@ def test_create_taxonomy(self) -> None: # Note: we must specify '-> None' to op assert not taxonomy.system_defined assert taxonomy.visible_to_authors + def test_create_taxonomy_without_export_id(self) -> None: + params: dict[str, Any] = { + "name": "Taxonomy Data: test 3", + "enabled": False, + "allow_multiple": True, + "allow_free_text": True, + } + taxonomy = tagging_api.create_taxonomy(**params) + assert taxonomy.export_id == "7-taxonomy-data-test-3" + def test_bad_taxonomy_class(self) -> None: with self.assertRaises(ValueError) as exc: tagging_api.create_taxonomy( @@ -80,8 +90,8 @@ def test_get_taxonomy(self) -> None: assert no_tax is None def test_get_taxonomies(self) -> None: - tax1 = tagging_api.create_taxonomy("Enabled", "enabled") - tax2 = tagging_api.create_taxonomy("Disabled", "disabled", enabled=False) + tax1 = tagging_api.create_taxonomy("Enabled") + tax2 = tagging_api.create_taxonomy("Disabled", enabled=False) tax3 = Taxonomy.objects.get(name="Import Taxonomy Test") with self.assertNumQueries(1): enabled = list(tagging_api.get_taxonomies()) @@ -582,7 +592,7 @@ def test_get_object_tags_deleted_disabled(self) -> None: obj_id = "object_id1" self.taxonomy.allow_multiple = True self.taxonomy.save() - disabled_taxonomy = tagging_api.create_taxonomy("Disabled Taxonomy", "disabled_taxonomy", allow_free_text=True) + disabled_taxonomy = tagging_api.create_taxonomy("Disabled Taxonomy", allow_free_text=True) tagging_api.tag_object(object_id=obj_id, taxonomy=self.taxonomy, tags=["DPANN", "Chordata"]) tagging_api.tag_object(object_id=obj_id, taxonomy=self.language_taxonomy, tags=["English"]) tagging_api.tag_object(object_id=obj_id, taxonomy=self.free_text_taxonomy, tags=["has a notochord"]) diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index abfd1a528..904277021 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -80,7 +80,7 @@ def create_sort_test_taxonomy(self) -> Taxonomy: Helper method to create a taxonomy that's difficult to sort correctly in tree order. """ # pylint: disable=unused-variable - taxonomy = api.create_taxonomy("Sort Test", "sort_test") + taxonomy = api.create_taxonomy("Sort Test") root1 = Tag.objects.create(taxonomy=taxonomy, value="1") child1_1 = Tag.objects.create(taxonomy=taxonomy, value="11", parent=root1) diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 4d95ccb7b..54509c5e5 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -75,15 +75,6 @@ def check_taxonomy( assert data["export_id"] == export_id -def create_taxonomy(name, description=None, enabled=True): - return api.create_taxonomy( - name, - export_id=name.lower().replace(" ", "_"), - description=description, - enabled=enabled, - ) - - class TestTaxonomyViewMixin(APITestCase): """ Mixin for taxonomy views. Adds users. @@ -125,9 +116,9 @@ class TestTaxonomyViewSet(TestTaxonomyViewMixin): ) @ddt.unpack def test_list_taxonomy_queryparams(self, enabled, expected_status: int, expected_count: int | None): - create_taxonomy(name="Taxonomy enabled 1", enabled=True) - create_taxonomy(name="Taxonomy enabled 2", enabled=True) - create_taxonomy(name="Taxonomy disabled", enabled=False) + api.create_taxonomy(name="Taxonomy enabled 1", enabled=True) + api.create_taxonomy(name="Taxonomy enabled 2", enabled=True) + api.create_taxonomy(name="Taxonomy disabled", enabled=False) url = TAXONOMY_LIST_URL @@ -150,7 +141,7 @@ def test_list_taxonomy_queryparams(self, enabled, expected_status: int, expected ) @ddt.unpack def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_count: int, is_admin=False): - taxonomy = create_taxonomy(name="Taxonomy enabled 1", enabled=True) + taxonomy = api.create_taxonomy(name="Taxonomy enabled 1", enabled=True) for i in range(tags_count): tag = Tag( taxonomy=taxonomy, @@ -184,7 +175,7 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c "can_change_taxonomy": False, "can_delete_taxonomy": False, "can_tag_object": False, - "export_id": "-1_languages", + "export_id": "-1-languages", }, { "id": taxonomy.id, @@ -202,18 +193,18 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c # can_tag_object is False because we default to not allowing users to tag arbitrary objects. # But specific uses of this code (like content_tagging) will override this perm for their use cases. "can_tag_object": False, - "export_id": "taxonomy_enabled_1", + "export_id": "2-taxonomy-enabled-1", }, ] assert response.data.get("can_add_taxonomy") == is_admin def test_list_taxonomy_pagination(self) -> None: url = TAXONOMY_LIST_URL - create_taxonomy(name="T1", enabled=True) - create_taxonomy(name="T2", enabled=True) - create_taxonomy(name="T3", enabled=False) - create_taxonomy(name="T4", enabled=False) - create_taxonomy(name="T5", enabled=False) + api.create_taxonomy(name="T1", enabled=True) + api.create_taxonomy(name="T2", enabled=True) + api.create_taxonomy(name="T3", enabled=False) + api.create_taxonomy(name="T4", enabled=False) + api.create_taxonomy(name="T5", enabled=False) self.client.force_authenticate(user=self.staff) @@ -260,8 +251,8 @@ def test_list_taxonomy_query_count(self): """ Test how many queries are used when retrieving taxonomies and permissions """ - create_taxonomy(name="T1", enabled=True) - create_taxonomy(name="T2", enabled=True) + api.create_taxonomy(name="T1", enabled=True) + api.create_taxonomy(name="T2", enabled=True) url = TAXONOMY_LIST_URL @@ -307,7 +298,7 @@ def test_language_taxonomy(self): can_change_taxonomy=False, can_delete_taxonomy=False, can_tag_object=False, - export_id='-1_languages', + export_id='-1-languages', ) @ddt.data( @@ -400,9 +391,31 @@ def test_create_taxonomy(self, user_attr: str | None, expected_status: int): response = self.client.get(url) check_taxonomy(response.data, response.data["id"], **create_data) + def test_create_without_export_id(self): + url = TAXONOMY_LIST_URL + + create_data = { + "name": "Taxonomy Data 3", + "description": "This is a description", + "enabled": False, + "allow_multiple": True, + } + + self.client.force_authenticate(user=self.staff) + response = self.client.post(url, create_data, format="json") + assert response.status_code == status.HTTP_201_CREATED + create_data["can_change_taxonomy"] = True + create_data["can_delete_taxonomy"] = True + create_data["can_tag_object"] = False + check_taxonomy( + response.data, + response.data["id"], + export_id="2-taxonomy-data-3", + **create_data, + ) + @ddt.data( {}, - {"name": "Error taxonomy"}, # Without export_id {"name": "Error taxonomy 1", "export_id": "Invalid value"}, # Invalid export_id {"name": "Error taxonomy 3", "export_id": "test", "enabled": "Invalid value"}, ) @@ -432,7 +445,7 @@ def test_create_taxonomy_system_defined(self, create_data): ) @ddt.unpack def test_update_taxonomy(self, user_attr, expected_status): - taxonomy = create_taxonomy( + taxonomy = api.create_taxonomy( name="test update taxonomy", description="taxonomy description", enabled=True, @@ -460,7 +473,7 @@ def test_update_taxonomy(self, user_attr, expected_status): "can_change_taxonomy": True, "can_delete_taxonomy": True, "can_tag_object": False, - "export_id": "test_update_taxonomy", + "export_id": "2-test-update-taxonomy", }, ) @@ -475,7 +488,6 @@ def test_update_taxonomy_system_defined(self, system_defined, expected_status): """ taxonomy = api.create_taxonomy( name="test system taxonomy", - export_id="test_system_taxonomy", taxonomy_class=SystemDefinedTaxonomy if system_defined else None, ) url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) @@ -498,7 +510,7 @@ def test_update_taxonomy_404(self): ) @ddt.unpack def test_patch_taxonomy(self, user_attr, expected_status): - taxonomy = create_taxonomy(name="test patch taxonomy", enabled=False) + taxonomy = api.create_taxonomy(name="test patch taxonomy", enabled=False) url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) @@ -521,7 +533,7 @@ def test_patch_taxonomy(self, user_attr, expected_status): "can_change_taxonomy": True, "can_delete_taxonomy": True, "can_tag_object": False, - "export_id": 'test_patch_taxonomy', + "export_id": '2-test-patch-taxonomy', }, ) @@ -536,7 +548,6 @@ def test_patch_taxonomy_system_defined(self, system_defined, expected_status): """ taxonomy = api.create_taxonomy( name="test system taxonomy", - export_id="test_system_taxonomy", taxonomy_class=SystemDefinedTaxonomy if system_defined else None, ) url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) @@ -559,7 +570,7 @@ def test_patch_taxonomy_404(self): ) @ddt.unpack def test_delete_taxonomy(self, user_attr, expected_status): - taxonomy = create_taxonomy(name="test delete taxonomy") + taxonomy = api.create_taxonomy(name="test delete taxonomy") url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) @@ -591,7 +602,7 @@ def test_export_taxonomy(self, output_format, content_type): """ Tests if a user can export a taxonomy """ - taxonomy = create_taxonomy(name="T1") + taxonomy = api.create_taxonomy(name="T1") for i in range(20): # Valid ObjectTags Tag.objects.create(taxonomy=taxonomy, value=f"Tag {i}").save() @@ -618,7 +629,7 @@ def test_export_taxonomy_download(self, output_format, content_type): """ Tests if a user can export a taxonomy with download option """ - taxonomy = create_taxonomy(name="T1") + taxonomy = api.create_taxonomy(name="T1") for i in range(20): api.add_tag_to_taxonomy(taxonomy=taxonomy, tag=f"Tag {i}") @@ -640,7 +651,7 @@ def test_export_taxonomy_invalid_param_output_format(self): """ Tests if a user can export a taxonomy using an invalid output_format param """ - taxonomy = create_taxonomy(name="T1") + taxonomy = api.create_taxonomy(name="T1") url = TAXONOMY_EXPORT_URL.format(pk=taxonomy.pk) @@ -652,7 +663,7 @@ def test_export_taxonomy_invalid_param_download(self): """ Tests if a user can export a taxonomy using an invalid output_format param """ - taxonomy = create_taxonomy(name="T1") + taxonomy = api.create_taxonomy(name="T1") url = TAXONOMY_EXPORT_URL.format(pk=taxonomy.pk) @@ -665,7 +676,7 @@ def test_export_taxonomy_unauthorized(self): Tests if a user can export a taxonomy that he doesn't have authorization """ # Only staff can view a disabled taxonomy - taxonomy = create_taxonomy(name="T1", enabled=False) + taxonomy = api.create_taxonomy(name="T1", enabled=False) url = TAXONOMY_EXPORT_URL.format(pk=taxonomy.pk) @@ -2379,6 +2390,7 @@ def test_import(self, file_format: str) -> None: taxonomy = response.data assert taxonomy["name"] == "Imported Taxonomy name" assert taxonomy["description"] == "Imported Taxonomy description" + assert taxonomy["export_id"] == "imported_taxonomy_export_id" # Check if the tags were created url = TAXONOMY_TAGS_URL.format(pk=taxonomy["id"]) From 956ff9629c8dec56fea095e25cc81f899cafc10c Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Wed, 31 Jan 2024 12:17:20 -0500 Subject: [PATCH 7/8] style: Nit on tests --- tests/openedx_tagging/core/tagging/test_api.py | 1 - tests/openedx_tagging/core/tagging/test_views.py | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index bad649da0..73b82bc93 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -78,7 +78,6 @@ def test_bad_taxonomy_class(self) -> None: with self.assertRaises(ValueError) as exc: tagging_api.create_taxonomy( name="Bad class", - export_id="bad_class", taxonomy_class=str, # type: ignore[arg-type] ) assert " must be a subclass of Taxonomy" in str(exc.exception) diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 54509c5e5..82c43dc96 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -426,7 +426,7 @@ def test_create_taxonomy_error(self, create_data: dict[str, str]): response = self.client.post(url, create_data, format="json") assert response.status_code == status.HTTP_400_BAD_REQUEST - @ddt.data({"name": "System defined taxonomy", "export_id": "sd", "system_defined": True}) + @ddt.data({"name": "System defined taxonomy", "system_defined": True}) def test_create_taxonomy_system_defined(self, create_data): """ Cannont create a taxonomy with system_defined=true From f53025c0f6868443006abdc7a55ccd2a65149d1a Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Thu, 1 Feb 2024 13:08:14 -0500 Subject: [PATCH 8/8] style: Nits --- openedx_tagging/core/tagging/rest_api/v1/views.py | 2 +- tests/openedx_tagging/core/tagging/test_api.py | 8 +++----- tests/openedx_tagging/core/tagging/test_models.py | 7 +++---- tests/openedx_tagging/core/tagging/test_views.py | 5 +---- 4 files changed, 8 insertions(+), 14 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index acb45fa0d..5eed0e5ab 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -58,7 +58,7 @@ class TaxonomyView(ModelViewSet): """ View to list, create, retrieve, update, delete, export or import Taxonomies. - TODO: We need to add a perform_udate and call the api update function when is created. + TODO: We need to add a perform_update and call the api update function when is created. This is because it is necessary to call the model validations. (`full_clean()`). Currently those validations are not run, which means we could update `export_id` with any value through this api. diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 73b82bc93..9cfeec373 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -253,10 +253,9 @@ def test_get_children_tags_invalid_taxonomy(self) -> None: """ Calling get_children_tags on free text taxonomies gives an error. """ - free_text_taxonomy = Taxonomy.objects.create( - allow_free_text=True, + free_text_taxonomy = tagging_api.create_taxonomy( name="FreeText", - export_id="free_text" + allow_free_text=True, ) tagging_api.tag_object(object_id="obj1", taxonomy=free_text_taxonomy, tags=["some_tag"]) with self.assertRaises(ValueError) as exc: @@ -272,11 +271,10 @@ def test_get_children_tags_no_children(self) -> None: def test_resync_object_tags(self) -> None: self.taxonomy.allow_multiple = True self.taxonomy.save() - open_taxonomy = Taxonomy.objects.create( + open_taxonomy = tagging_api.create_taxonomy( name="Freetext Life", allow_free_text=True, allow_multiple=True, - export_id='freetext_life', ) object_id = "obj1" diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index 904277021..303e7e5fd 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -38,7 +38,7 @@ def setUp(self): self.system_taxonomy = Taxonomy.objects.get(name="System defined taxonomy") self.language_taxonomy = LanguageTaxonomy.objects.get(name="Languages") self.user_taxonomy = Taxonomy.objects.get(name="User Authors").cast() - self.free_text_taxonomy = Taxonomy.objects.create(name="Free Text", allow_free_text=True) + self.free_text_taxonomy = api.create_taxonomy(name="Free Text", allow_free_text=True) # References to some tags: self.archaea = get_tag("Archaea") @@ -111,11 +111,10 @@ def create_100_taxonomies(self): """ dummy_taxonomies = [] for i in range(100): - taxonomy = Taxonomy.objects.create( + taxonomy = api.create_taxonomy( name=f"ZZ Dummy Taxonomy {i:03}", allow_free_text=True, allow_multiple=True, - export_id=f"zz_dummy_taxonomy_{i:03}" ) ObjectTag.objects.create( object_id="limit_tag_count", @@ -602,7 +601,7 @@ class TestFilteredTagsFreeTextTaxonomy(TestCase): def setUp(self): super().setUp() - self.taxonomy = Taxonomy.objects.create(allow_free_text=True, name="FreeText") + self.taxonomy = api.create_taxonomy(allow_free_text=True, name="FreeText") # The "triple" tag will be applied to three objects, "double" to two, and "solo" to one: api.tag_object(object_id="obj1", taxonomy=self.taxonomy, tags=["triple"]) api.tag_object(object_id="obj2", taxonomy=self.taxonomy, tags=["triple", "double"]) diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 82c43dc96..d29f56fd5 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -2544,10 +2544,7 @@ def _check_taxonomy_not_changed(self) -> None: def setUp(self): ImportTaxonomyMixin.setUp(self) - self.taxonomy = Taxonomy.objects.create( - name="Test import taxonomy", - export_id="test_import_taxonomy", - ) + self.taxonomy = api.create_taxonomy(name="Test import taxonomy") tag_1 = Tag.objects.create( taxonomy=self.taxonomy, external_id="old_tag_1",