Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add export_id field on Taxonomy #145

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion openedx_learning/__init__.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
"""
Open edX Learning ("Learning Core").
"""
__version__ = "0.4.4"
__version__ = "0.5.0"
4 changes: 4 additions & 0 deletions openedx_tagging/core/tagging/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

def create_taxonomy(
name: str,
export_id: str,
description: str | None = None,
enabled=True,
allow_multiple=True,
Expand All @@ -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()

Expand Down
2 changes: 1 addition & 1 deletion openedx_tagging/core/tagging/import_export/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generated a warning on the lint

# Log any exception
task.log_exception(exception)
return False, task, None
Expand Down
37 changes: 37 additions & 0 deletions openedx_tagging/core/tagging/migrations/0015_taxonomy_export_id.py
Original file line number Diff line number Diff line change
@@ -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="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),
# Alter the field to not allowing null
migrations.AlterField(
model_name='taxonomy',
name='export_id',
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),
),
]
23 changes: 23 additions & 0 deletions openedx_tagging/core/tagging/models/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from __future__ import annotations

import logging
import re
from typing import List

from django.core.exceptions import ValidationError
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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

Expand Down
12 changes: 12 additions & 0 deletions openedx_tagging/core/tagging/rest_api/v1/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
ChrisChV marked this conversation as resolved.
Show resolved Hide resolved

def to_representation(self, instance):
"""
Cast the taxonomy before serialize
Expand Down Expand Up @@ -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):
Expand Down
21 changes: 19 additions & 2 deletions openedx_tagging/core/tagging/rest_api/v1/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
pomegranited marked this conversation as resolved.
Show resolved Hide resolved
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.

pomegranited marked this conversation as resolved.
Show resolved Hide resolved
**List Query Parameters**
* enabled (optional) - Filter by enabled status. Valid values: true,
false, 1, 0, "true", "false", "1"
Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand All @@ -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.
Expand All @@ -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,
Expand Down Expand Up @@ -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": <file>,
}
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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)

Expand Down
5 changes: 4 additions & 1 deletion tests/openedx_tagging/core/fixtures/tagging.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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
Expand All @@ -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
Expand All @@ -256,4 +259,4 @@
enabled: true
allow_multiple: false
allow_free_text: false

export_id: import_taxonomy_test
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
21 changes: 16 additions & 5 deletions tests/openedx_tagging/core/tagging/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -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():
Expand All @@ -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 "<class 'str'> must be a subclass of Taxonomy" in str(exc.exception)
Expand All @@ -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())
Expand Down Expand Up @@ -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(
pomegranited marked this conversation as resolved.
Show resolved Hide resolved
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")
Expand All @@ -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:
Expand Down Expand Up @@ -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"])
Expand Down
Loading
Loading