Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 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
29 changes: 29 additions & 0 deletions openedx_tagging/core/tagging/rest_api/v1/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from rest_framework.reverse import reverse

from openedx_tagging.core.tagging.data import TagData
from openedx_tagging.core.tagging.import_export.parsers import ParserFormat
from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy


Expand Down Expand Up @@ -175,3 +176,31 @@ class TaxonomyTagDeleteBodySerializer(serializers.Serializer): # pylint: disabl
child=serializers.CharField(), required=True
)
with_subtags = serializers.BooleanField(required=False)


class TaxonomyImportBodySerializer(serializers.Serializer): # pylint: disable=abstract-method
"""
Serializer of the body for the Taxonomy Import action
Comment thread
rpenido marked this conversation as resolved.
Outdated
"""
file = serializers.FileField(required=True)

def validate(self, attrs):
"""
Validates the file extension and add parser_format to the data
"""
filename = attrs["file"].name
ext = filename.split('.')[-1]
parser_format = getattr(ParserFormat, ext.upper(), None)
if not parser_format:
raise serializers.ValidationError({"file": f'File type not supported: {ext.lower()}'}, 'file')

attrs['parser_format'] = parser_format
return attrs


class TaxonomyImportNewBodySerializer(TaxonomyImportBodySerializer): # pylint: disable=abstract-method
"""
Serializer of the body for the Taxonomy Create and Import action
Comment thread
rpenido marked this conversation as resolved.
Outdated
"""
taxonomy_name = serializers.CharField(required=True)
taxonomy_description = serializers.CharField(default="")
86 changes: 84 additions & 2 deletions openedx_tagging/core/tagging/rest_api/v1/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@
from __future__ import annotations

from django.db import models
from django.http import Http404, HttpResponse
from django.http import Http404, HttpResponse, HttpResponseBadRequest
from rest_framework import mixins, status
from rest_framework.decorators import action
from rest_framework.exceptions import MethodNotAllowed, PermissionDenied, ValidationError
from rest_framework.generics import ListAPIView, RetrieveUpdateDestroyAPIView
from rest_framework.request import Request
from rest_framework.response import Response
from rest_framework.viewsets import GenericViewSet, ModelViewSet

Expand All @@ -24,7 +25,7 @@
update_tag_in_taxonomy,
)
from ...data import TagDataQuerySet
from ...import_export.api import export_tags
from ...import_export.api import export_tags, get_last_import_log, import_tags
from ...import_export.parsers import ParserFormat
from ...models import Taxonomy
from ...rules import ObjectTagPermissionItem
Expand All @@ -37,6 +38,8 @@
ObjectTagUpdateQueryParamsSerializer,
TagDataSerializer,
TaxonomyExportQueryParamsSerializer,
TaxonomyImportBodySerializer,
TaxonomyImportNewBodySerializer,
TaxonomyListQueryParamsSerializer,
TaxonomySerializer,
TaxonomyTagCreateBodySerializer,
Expand Down Expand Up @@ -164,7 +167,29 @@ class TaxonomyView(ModelViewSet):
* 400 - Invalid query parameter
* 403 - Permission denied

**Import/Create Taxonomy Example Requests**
POST /tagging/rest_api/v1/taxonomy/import/
{
"taxonomy_name": "Taxonomy Name",
"taxonomy_description": "This is a description",
"file": <file>,
}

**Import/Create Taxonomy Query Returns**
* 200 - Success
* 400 - Bad request
* 403 - Permission denied

**Import/Update Taxonomy Example Requests**
PUT /tagging/rest_api/v1/taxonomy/:pk/tags/import/
{
"file": <file>,
}

**Import/Update Taxonomy Query Returns**
* 200 - Success
* 400 - Bad request
* 403 - Permission denied
"""

lookup_value_regex = r"\d+"
Expand Down Expand Up @@ -239,6 +264,63 @@ def export(self, request, **_kwargs) -> HttpResponse:

return HttpResponse(tags, content_type=content_type)

@action(detail=False, url_path="import", methods=["post"])
def create_import(self, request: Request, **_kwargs) -> HttpResponse:
"""
Create a new taxonomy and imports the tags from the uploaded file.
"""
perm = "oel_tagging.import_taxonomy"
if not request.user.has_perm(perm):
raise PermissionDenied("You do not have permission to import taxonomies")
Comment thread
pomegranited marked this conversation as resolved.
Outdated

body = TaxonomyImportNewBodySerializer(data=request.data)
body.is_valid(raise_exception=True)

taxonomy_name = body.validated_data["taxonomy_name"]
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)
try:
import_success = import_tags(taxonomy, file, parser_format)

if import_success:
return HttpResponse(status=200)
Comment thread
pomegranited marked this conversation as resolved.
Outdated
else:
import_error = get_last_import_log(taxonomy)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm.. This works fine in a serial environment, but if we've got multiple imports happening at the same time, this won't necessarily be the import log we're interested in here. Really, the only way to get that is if we have the task ID after import.

But I don't know if it's worth refactoring import_tags to support this, so: could you add a comment here to warn us? Then we'll know where to look if users report problems.

@rpenido rpenido Nov 8, 2023 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is not as alarming as I made it sound:
1- It only happens on update. If we do a mass import/create we will have new ids, so no problem.
2- I did another look at the code and realized that we prevent multiple imports of the same taxonomy simultaneously. So, the concurrent problem may not occur at all, right?

taxonomy.delete()
return HttpResponseBadRequest(import_error)
except ValueError as e:
return HttpResponseBadRequest(e)

@action(detail=True, url_path="tags/import", methods=["put"])
def update_import(self, request: Request, **_kwargs) -> HttpResponse:
"""
Imports tags from the uploaded file to an already created taxonomy.
"""
perm = "oel_tagging.import_taxonomy"
if not request.user.has_perm(perm):
raise PermissionDenied("You do not have permission to import taxonomies")
Comment thread
pomegranited marked this conversation as resolved.
Outdated

body = TaxonomyImportBodySerializer(data=request.data)
body.is_valid(raise_exception=True)

file = body.validated_data["file"].file
parser_format = body.validated_data["parser_format"]

taxonomy = self.get_object()
try:
import_success = import_tags(taxonomy, file, parser_format)

if import_success:
return HttpResponse(status=200)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ditto here, can we return the Taxonomy on success?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I think we will need to revisit this in the future.

I think we should return the Taxonomy for now, but we will probably want to return the tag list for this taxonomy (at least the first page of it) after the import.

We will have a cleaner view when implementing the Import inside the Taxonomy Details.

And just to make sure: the import inside the Taxonomy Details is not part of this task, right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rpenido

We will have a cleaner view when implementing the Import inside the Taxonomy Details.

Agreed -- ok to just return Taxonomy for now.

And just to make sure: the import inside the Taxonomy Details is not part of this task, right?

Correct, importing tags to an existing taxonomy is covered by openedx/modular-learning#140

else:
import_error = get_last_import_log(taxonomy)
return HttpResponseBadRequest(import_error)
except ValueError as e:
return HttpResponseBadRequest(e)


@view_auth_classes
class ObjectTagView(
Expand Down
1 change: 1 addition & 0 deletions openedx_tagging/core/tagging/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ def can_change_object_tag(
rules.add_perm("oel_tagging.delete_taxonomy", can_change_taxonomy)
rules.add_perm("oel_tagging.view_taxonomy", can_view_taxonomy)
rules.add_perm("oel_tagging.export_taxonomy", can_view_taxonomy)
rules.add_perm("oel_tagging.import_taxonomy", can_change_taxonomy)
Comment thread
pomegranited marked this conversation as resolved.
Outdated

# Tag
rules.add_perm("oel_tagging.add_tag", can_change_tag)
Expand Down
Loading