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 8 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"
8 changes: 8 additions & 0 deletions openedx_tagging/core/tagging/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -34,19 +35,26 @@ def create_taxonomy(
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 "",
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
38 changes: 38 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,38 @@
# 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}-{slugify(taxonomy.name, allow_unicode=True)}"
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
3 changes: 3 additions & 0 deletions openedx_tagging/core/tagging/rest_api/v1/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -88,6 +89,7 @@ class Meta:
"can_change_taxonomy",
"can_delete_taxonomy",
"can_tag_object",
"export_id",
]

def to_representation(self, instance):
Expand Down Expand Up @@ -332,6 +334,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_description, export_id=taxonomy_export_id)
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
24 changes: 22 additions & 2 deletions tests/openedx_tagging/core/tagging/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,24 @@ 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():
assert getattr(taxonomy, param) == value
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(
Expand Down Expand Up @@ -242,7 +253,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 +272,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
Loading
Loading