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 import taxonomy endpoint #33663

Merged
Show file tree
Hide file tree
Changes from 3 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

Large diffs are not rendered by default.

81 changes: 80 additions & 1 deletion openedx/core/djangoapps/content_tagging/rest_api/v1/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,14 @@
Tagging Org API Views
"""

from django.http import HttpResponse, HttpResponseBadRequest
from openedx_tagging.core.tagging import rules as oel_tagging_rules
from openedx_tagging.core.tagging.rest_api.v1.views import ObjectTagView, TaxonomyView

from openedx_tagging.core.tagging.rest_api.v1.serializers import TaxonomyImportBodySerializer, TaxonomyImportNewBodySerializer
from openedx_tagging.core.tagging.import_export.api import get_last_import_log, import_tags
from rest_framework.decorators import action
from rest_framework.exceptions import PermissionDenied
from rest_framework.request import Request

from ...api import (
create_taxonomy,
Expand Down Expand Up @@ -61,6 +67,79 @@ def perform_create(self, serializer):
user_admin_orgs = get_admin_orgs(self.request.user)
serializer.instance = create_taxonomy(**serializer.validated_data, orgs=user_admin_orgs)

@action(detail=False, url_path="import", methods=["post"])
def create_import(self, request: Request, **_kwargs) -> HttpResponse:
"""
Creates 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")

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"]

# ToDo: This code is temporary
# In the future, the orgs parameter will be defined in the request body from the frontend
# See: https://github.com/openedx/modular-learning/issues/116
if oel_tagging_rules.is_taxonomy_admin(request.user):
orgs = None
else:
orgs = get_admin_orgs(request.user)

taxonomy = create_taxonomy(taxonomy_name, taxonomy_description, orgs=orgs)
try:
import_success = import_tags(taxonomy, file, parser_format)

if import_success:
return HttpResponse(status=200)
else:
import_error = get_last_import_log(taxonomy)
taxonomy.delete()
return HttpResponseBadRequest(import_error)
except ValueError as e:
return HttpResponseBadRequest(e)
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that the parent TaxonomyView returns the created taxonomy, we can do this with less duplicated code:

Suggested change
@action(detail=False, url_path="import", methods=["post"])
def create_import(self, request: Request, **_kwargs) -> HttpResponse:
"""
Creates 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")
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"]
# ToDo: This code is temporary
# In the future, the orgs parameter will be defined in the request body from the frontend
# See: https://github.com/openedx/modular-learning/issues/116
if oel_tagging_rules.is_taxonomy_admin(request.user):
orgs = None
else:
orgs = get_admin_orgs(request.user)
taxonomy = create_taxonomy(taxonomy_name, taxonomy_description, orgs=orgs)
try:
import_success = import_tags(taxonomy, file, parser_format)
if import_success:
return HttpResponse(status=200)
else:
import_error = get_last_import_log(taxonomy)
taxonomy.delete()
return HttpResponseBadRequest(import_error)
except ValueError as e:
return HttpResponseBadRequest(e)
@action(detail=False, url_path="import", methods=["post"])
def create_import(self, request: Request, **kwargs) -> HttpResponse:
"""
Creates a new taxonomy with the given orgs and imports the tags from the uploaded file.
"""
response = super().create_import(request, **kwargs)
# If creation was successful, set the orgs for the new taxonomy
if status.is_success(response.status_code):
# ToDo: This code is temporary
# In the future, the orgs parameter will be defined in the request body from the frontend
# See: https://github.com/openedx/modular-learning/issues/116
if oel_tagging_rules.is_taxonomy_admin(request.user):
orgs = None
else:
orgs = get_admin_orgs(request.user)
taxonomy = get_taxonomy(response.data['id']) # or wherever it is
set_taxonomy_orgs(taxonomy, all_orgs=False, orgs=orgs)
# might need to re-serialize the response to include the orgs?
return response

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about this one @pomegranited.

We are preventing some code duplication in the View but duplicating the logic of the edx-platform/create_taxonomy.

It will be a pain if our create_taxonomy gets complicated. Or we will get in trouble if we change some behavior from the oel_tagging/create_taxonomy.

Do you think it is ok to leave the duplicated create_import?

Copy link
Contributor

@pomegranited pomegranited Nov 10, 2023

Choose a reason for hiding this comment

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

@rpenido I don't think we should be writing/maintaining more code than we need..

@bradenmacdonald , could we get a 2nd opinion here?

Copy link
Contributor

@bradenmacdonald bradenmacdonald Nov 10, 2023

Choose a reason for hiding this comment

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

I like @pomegranited's suggestion; it re-uses more code. If we're going to have a separate "create_import" function at all, we should aim to keep it as small as possible, just calling the other create and import functions.

The content_tagging version of create_taxonomy is only two lines of code so it's fine to duplicate it here. Just make sure to add comments explaining everything and any concerns about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the discussion! 😄

Done here: 1b65728


@action(detail=True, url_path="tags/import", methods=["put"])
def update_import(self, request: Request, **_kwargs) -> HttpResponse:
"""
Creates 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")

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

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

# ToDo: This code is temporary
# In the future, the orgs parameter will be defined in the request body from the frontend
# See: https://github.com/openedx/modular-learning/issues/116
if oel_tagging_rules.is_taxonomy_admin(request.user):
orgs = None
else:
orgs = get_admin_orgs(request.user)

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

if import_success:
return HttpResponse(status=200)
else:
import_error = get_last_import_log(taxonomy)
return HttpResponseBadRequest(import_error)
except ValueError as e:
return HttpResponseBadRequest(e)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can this whole method can go away? I think the parent TaxonomyView now handles all that's needed here.

Suggested change
@action(detail=True, url_path="tags/import", methods=["put"])
def update_import(self, request: Request, **_kwargs) -> HttpResponse:
"""
Creates 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")
body = TaxonomyImportBodySerializer(data=request.data)
body.is_valid(raise_exception=True)
file = body.validated_data["file"].file
parser_format = body.validated_data["parser_format"]
# ToDo: This code is temporary
# In the future, the orgs parameter will be defined in the request body from the frontend
# See: https://github.com/openedx/modular-learning/issues/116
if oel_tagging_rules.is_taxonomy_admin(request.user):
orgs = None
else:
orgs = get_admin_orgs(request.user)
taxonomy = self.get_object()
try:
import_success = import_tags(taxonomy, file, parser_format)
if import_success:
return HttpResponse(status=200)
else:
import_error = get_last_import_log(taxonomy)
return HttpResponseBadRequest(import_error)
except ValueError as e:
return HttpResponseBadRequest(e)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done ea67de1!


class ObjectTagOrgView(ObjectTagView):
"""
Expand Down
1 change: 1 addition & 0 deletions openedx/core/djangoapps/content_tagging/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,7 @@ def can_change_taxonomy_tag(user: UserType, tag: oel_tagging.Tag | None = None)
rules.set_perm("oel_tagging.delete_taxonomy", can_change_taxonomy)
rules.set_perm("oel_tagging.view_taxonomy", can_view_taxonomy)
rules.set_perm("oel_tagging.export_taxonomy", can_view_taxonomy)
rules.set_perm("oel_tagging.import_taxonomy", can_create_taxonomy)
rpenido marked this conversation as resolved.
Show resolved Hide resolved

# Tag
rules.set_perm("oel_tagging.add_tag", can_change_taxonomy_tag)
Expand Down
2 changes: 1 addition & 1 deletion requirements/constraints.txt
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ libsass==0.10.0
click==8.1.6

# pinning this version to avoid updates while the library is being developed
openedx-learning==0.3.0
openedx-learning==0.3.1

# lti-consumer-xblock 9.6.2 contains a breaking change that makes
# existing custom parameter configurations unusable.
Expand Down
2 changes: 1 addition & 1 deletion requirements/edx/base.txt
Original file line number Diff line number Diff line change
Expand Up @@ -785,7 +785,7 @@ openedx-filters==1.6.0
# via
# -r requirements/edx/kernel.in
# lti-consumer-xblock
openedx-learning==0.3.0
openedx-learning @ git+https://github.com/open-craft/openedx-learning@rpenido/fal-3532-import-taxonomy-bare-bones
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/kernel.in
Expand Down
2 changes: 1 addition & 1 deletion requirements/edx/development.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1315,7 +1315,7 @@ openedx-filters==1.6.0
# -r requirements/edx/doc.txt
# -r requirements/edx/testing.txt
# lti-consumer-xblock
openedx-learning==0.3.0
openedx-learning @ git+https://github.com/open-craft/openedx-learning@rpenido/fal-3532-import-taxonomy-bare-bones
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/doc.txt
Expand Down
2 changes: 1 addition & 1 deletion requirements/edx/doc.txt
Original file line number Diff line number Diff line change
Expand Up @@ -925,7 +925,7 @@ openedx-filters==1.6.0
# via
# -r requirements/edx/base.txt
# lti-consumer-xblock
openedx-learning==0.3.0
openedx-learning @ git+https://github.com/open-craft/openedx-learning@rpenido/fal-3532-import-taxonomy-bare-bones
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/base.txt
Expand Down
3 changes: 2 additions & 1 deletion requirements/edx/kernel.in
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,8 @@ openedx-calc # Library supporting mathematical calculatio
openedx-django-require
openedx-events # Open edX Events from Hooks Extension Framework (OEP-50)
openedx-filters # Open edX Filters from Hooks Extension Framework (OEP-50)
openedx-learning # Open edX Learning core (experimental)
# openedx-learning # Open edX Learning core (experimental)
openedx-learning @ git+https://github.com/open-craft/openedx-learning@rpenido/fal-3532-import-taxonomy-bare-bones
openedx-mongodbproxy
openedx-django-wiki
openedx-blockstore
Expand Down
2 changes: 1 addition & 1 deletion requirements/edx/testing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -989,7 +989,7 @@ openedx-filters==1.6.0
# via
# -r requirements/edx/base.txt
# lti-consumer-xblock
openedx-learning==0.3.0
openedx-learning @ git+https://github.com/open-craft/openedx-learning@rpenido/fal-3532-import-taxonomy-bare-bones
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/base.txt
Expand Down