feat: add import taxonomy endpoint #112

rpenido · 2023-11-01T20:51:38Z

Description

Add a rest api endpoint to import taxonomies

Supporting Information

Part of [Tagging] Import a Taxonomy ("Bare Bones") modular-learning#115

Testing instructions

Please ensure that the tests cover the expected behavior of the view

Private-ref: FAL-3532

openedx-webhooks · 2023-11-01T20:51:43Z

Thanks for the pull request, @rpenido! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

openedx_tagging/core/tagging/rest_api/v1/serializers.py

pomegranited · 2023-11-02T06:23:45Z

openedx_tagging/core/tagging/rest_api/v1/views_import.py

+            taxonomy_description=taxonomy_description,
+            file=file,
+            parser_format=parser_format,
+        )


Hmm.. the import/export tags API was designed to use async celery tasks so it could run in the background (in case there's a lot of tags to import).

@bradenmacdonald do you want us to use these async tasks here, or are we doing all imports synchronously?

I suspect synchronous will be totally fine for the short term, but we might as well do it async since we've done most of the work already. How much work will that add to make the REST API layer async too?

What do we mean to make the call async here?

The client makes the API call, gets the response "Job running.." immediately, and later is notified (websocket or pooling) that the job is done and the results

The client makes the API call, and the REST API calls the Python API async but waits for the result before sending the response to the client (the HTTP request will be "sync", but we will use the celery task)

The 2 is simple (but I don't know if we have some gain that way).
1 size depends on what we want to accomplish. Will we just show a toast, or we want a page listing the job history?

I meant (1). But with simple polling for now, no need for websockets or anything fancy. For (2) it actually ties up much more server resources than just directly doing the import synchronously.

If that's going to take a while to implement though we can just to sync for now.

Got it!
There is only one problem in our flow:
We need to create the Taxonomy before calling the import.

If the import fails, it is easy to remove the created taxonomy in the sync call.

Doing async, we can create the taxonomy, return the id to the client, and make calls waiting for the results (the result is associated with the taxonomy). But I need to figure out how to delete the freshly created taxonomy after a failed attempt. Would you happen to have any ideas on this?

This is not urgent for this task, but we will need to handle it if we want to go async sometime in the future. One approach is to change the import flow in the front-end: we first create the taxonomy and always call import inside it (then we don't need to handle the delete). This will impact our designs and this task a bit.

Another approach is to handle the create+import sync and use async for importing on an already created taxonomy. But it will also impact the UX, having different flows.

I think the appropriate solution will be to refactor the import to let it create the taxonomy and change the results to be tied to some kind of "job-id", and not the taxonomy itself.

I think the right approach is to create an additional async task which creates the taxonomy, then synchronously calls the existing import task (which is a celery task so can be called either sync or async, but since we're already in an async task, we call it sync). If that succeeds, it returns the taxonomy ID, and if it fails, it deletes the taxonomy and returns an error message.

The main thing is that this "wrapper" task which does create+import returns an import ID not a taxonomy ID. And then the frontend polls the import ID until it gets either a success or failure message. Only then does it get the taxonomy ID.

But that's all getting too complicated. Just do it synchronously for now an we'll make it async if it's too slow in practice. Explicitly put a comment in the REST API for the import that "this is an unstable API and may change if we make it async."

Co-authored-by: Jillian <[email protected]>

rpenido · 2023-11-06T13:49:03Z

I think this is ready for a ~~final~~ review @pomegranited !

But I prefer to wait for the development of the edx-platform part before merging.

openedx_tagging/core/tagging/rest_api/v1/serializers.py

openedx_tagging/core/tagging/import_export/api.py

openedx_tagging/core/tagging/rest_api/v1/views_import.py

openedx_tagging/core/tagging/import_export/api.py

openedx_tagging/core/tagging/rest_api/v1/views.py

pomegranited

This is looking good, and the tests look great @rpenido! Comments inline.

openedx_tagging/core/tagging/rest_api/v1/views.py

pomegranited · 2023-11-07T23:14:26Z

openedx_tagging/core/tagging/rest_api/v1/views.py

+            import_success = import_tags(taxonomy, file, parser_format)
+
+            if import_success:
+                return HttpResponse(status=200)


Ditto here, can we return the Taxonomy on success?

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?

@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

pomegranited · 2023-11-07T23:16:36Z

openedx_tagging/core/tagging/rest_api/v1/views.py

+            if import_success:
+                return HttpResponse(status=200)
+            else:
+                import_error = get_last_import_log(taxonomy)


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.

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?

openedx_tagging/core/tagging/rules.py

…e-bones

rpenido · 2023-11-08T21:35:02Z

~~ToDo: Bump version before merge~~

pomegranited

👍 This looks great @rpenido ! Ready for your review @bradenmacdonald .

Note: needs a version bump before merging.

I tested this
I read through the code
~~I checked for accessibility issues~~ N/A
Includes documentation

bradenmacdonald · 2023-11-09T02:07:27Z

tests/openedx_tagging/core/tagging/test_views.py

+    def setUp(self):
+        ImportTaxonomyMixin.setUp(self)
+
+        self.taxonomy = Taxonomy.objects.create(


Nit: it's not a big deal, but I generally would prefer that we use the API methods api.create_taxonomy(...) and api.add_tag_to_taxonomy(...) instead of using the models directly. In this case for example, add_tag_to_taxonomy does a lot of additional validation of the created objects, which is good to have, but isn't happening here if we just create the models directly.

The only major exception is in test_models.py where we're actually testing the models; in that case obviously we should use the models directly.

Even in the setUp @bradenmacdonald?

A breaking change will prevent the tests from running and be trickier to diagnose, don't you think?

Well that's the thing - the API is not supposed to have breaking changes in general. But we may change the internal models. So if this test isn't specifically testing the internal models, it should use the API which should be more stable. But again, don't worry about it for now. What you have is fine.

bradenmacdonald · 2023-11-09T02:08:31Z

@pomegranited From a very quick look, this seems good to me. You're a CC for this repo so I'm happy for you to approve and merge it, unless you want me to do a second review?

pomegranited · 2023-11-09T03:31:33Z

@bradenmacdonald Could you do a 2nd review? I'm happy to merge this when it's ready.

openedx_tagging/core/tagging/rest_api/v1/serializers.py

bradenmacdonald · 2023-11-09T18:51:30Z

openedx_tagging/core/tagging/rest_api/v1/views.py

@@ -215,9 +240,6 @@ def export(self, request, **_kwargs) -> HttpResponse:
        Export a taxonomy.
        """
        taxonomy = self.get_object()
-        perm = "oel_tagging.export_taxonomy"
-        if not request.user.has_perm(perm, taxonomy):
-            raise PermissionDenied("You do not have permission to export this taxonomy.")


It's true that "if you can view the taxonomy, you can export it", but are we still checking that the user has "view" permission here? I don't see it. (Though it looks like it's covered by the tests. I'm just wondering where that check is happening).

The get_object() function handles that:

https://github.com/encode/django-rest-framework/blob/1db19f4b2d1500894448634852e02f73043382e4/rest_framework/generics.py#L103

tests/openedx_tagging/core/tagging/test_views.py

bradenmacdonald · 2023-11-09T18:58:26Z

@pomegranited It looks good to me! Just saw some minor things to clean up.

Co-authored-by: Braden MacDonald <[email protected]>

pomegranited · 2023-11-09T23:47:24Z

Thanks for fixing those docstrings @rpenido ! Could you push a version bump when you're done addressing @bradenmacdonald 's review, and I'll get this merged.

rpenido · 2023-11-09T23:59:06Z

Thanks for fixing those docstrings @rpenido ! Could you push a version bump when you're done addressing @bradenmacdonald 's review, and I'll get this merged.

Done c4fe95d!

openedx-webhooks · 2023-11-10T02:09:21Z

@rpenido 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

pomegranited · 2023-11-10T02:12:44Z

Version 0.3.3 released.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 1, 2023

rpenido force-pushed the rpenido/fal-3532-import-taxonomy-bare-bones branch from d552b92 to cb30030 Compare November 1, 2023 21:06

feat: add import taxonomy endpoint

c1d5944

rpenido force-pushed the rpenido/fal-3532-import-taxonomy-bare-bones branch from cb30030 to c1d5944 Compare November 1, 2023 21:22

style: fix pylint

05105bc

pomegranited reviewed Nov 2, 2023

View reviewed changes

rpenido and others added 2 commits November 3, 2023 09:39

refactor: improve get_parse_format implementation

a6919a1

Co-authored-by: Jillian <[email protected]>

test: add new tests

f030767

rpenido force-pushed the rpenido/fal-3532-import-taxonomy-bare-bones branch from 326cde6 to f030767 Compare November 3, 2023 15:04

rpenido marked this pull request as ready for review November 6, 2023 13:48

rpenido added 2 commits November 6, 2023 11:54

feat: add permission check

4fe2457

style: fix pylint

06efd55

pomegranited reviewed Nov 7, 2023

View reviewed changes

refactor: remove duplicated code

d400a54

rpenido force-pushed the rpenido/fal-3532-import-taxonomy-bare-bones branch from f6b6854 to 3f1e08e Compare November 7, 2023 13:18

refactor: import taxonomy view

a50e08f

rpenido force-pushed the rpenido/fal-3532-import-taxonomy-bare-bones branch from 3f1e08e to a50e08f Compare November 7, 2023 13:30

rpenido commented Nov 7, 2023

View reviewed changes

openedx_tagging/core/tagging/rest_api/v1/views.py Outdated Show resolved Hide resolved

rpenido commented Nov 7, 2023

View reviewed changes

openedx_tagging/core/tagging/rest_api/v1/views.py Outdated Show resolved Hide resolved

rpenido force-pushed the rpenido/fal-3532-import-taxonomy-bare-bones branch from 77290f6 to e975107 Compare November 7, 2023 13:44

style: fix pylint

e1b1789

rpenido force-pushed the rpenido/fal-3532-import-taxonomy-bare-bones branch from e975107 to e1b1789 Compare November 7, 2023 13:44

rpenido added 2 commits November 7, 2023 11:11

feat: add allow_free_text test and style fix

d9950f6

feat: add error messages to response

7450737

rpenido mentioned this pull request Nov 7, 2023

feat: add import taxonomy endpoint openedx/edx-platform#33663

Merged

pomegranited requested changes Nov 7, 2023

View reviewed changes

rpenido added 2 commits November 8, 2023 10:50

refactor: remove redudant permissions

ae76400

refactor: return taxonomy after import tags

62e2593

refactor: use rest api to get tags from taxonomies

a827178

rpenido requested a review from pomegranited November 8, 2023 15:21

rpenido and others added 2 commits November 8, 2023 12:32

Merge branch 'openedx:main' into rpenido/fal-3532-import-taxonomy-bar…

730b73f

…e-bones

docs: fix docstring

4e52c57

pomegranited approved these changes Nov 8, 2023

View reviewed changes

bradenmacdonald reviewed Nov 9, 2023

View reviewed changes

bradenmacdonald approved these changes Nov 9, 2023

View reviewed changes

docs: update docstring

a8a0c7e

Co-authored-by: Braden MacDonald <[email protected]>

chore: bump version

c4fe95d

pomegranited merged commit 579790d into openedx:main Nov 10, 2023
7 checks passed

pomegranited deleted the rpenido/fal-3532-import-taxonomy-bare-bones branch November 10, 2023 02:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add import taxonomy endpoint #112

feat: add import taxonomy endpoint #112

rpenido commented Nov 1, 2023 •

edited

Loading

openedx-webhooks commented Nov 1, 2023 •

edited

Loading

pomegranited Nov 2, 2023

bradenmacdonald Nov 2, 2023

rpenido Nov 3, 2023

bradenmacdonald Nov 3, 2023

rpenido Nov 3, 2023 •

edited

Loading

bradenmacdonald Nov 3, 2023

rpenido commented Nov 6, 2023 •

edited

Loading

pomegranited left a comment

pomegranited Nov 7, 2023

rpenido Nov 8, 2023

pomegranited Nov 9, 2023

pomegranited Nov 7, 2023

rpenido Nov 8, 2023 •

edited

Loading

rpenido commented Nov 8, 2023 •

edited

Loading

pomegranited left a comment

bradenmacdonald Nov 9, 2023

rpenido Nov 9, 2023

bradenmacdonald Nov 9, 2023

bradenmacdonald commented Nov 9, 2023

pomegranited commented Nov 9, 2023

bradenmacdonald Nov 9, 2023 •

edited

Loading

rpenido Nov 9, 2023

bradenmacdonald commented Nov 9, 2023

pomegranited commented Nov 9, 2023

rpenido commented Nov 9, 2023

openedx-webhooks commented Nov 10, 2023

pomegranited commented Nov 10, 2023

feat: add import taxonomy endpoint #112

feat: add import taxonomy endpoint #112

Conversation

rpenido commented Nov 1, 2023 • edited Loading

Description

Supporting Information

Testing instructions

openedx-webhooks commented Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido commented Nov 6, 2023 • edited Loading

pomegranited left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

rpenido commented Nov 8, 2023 • edited Loading

pomegranited left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald commented Nov 9, 2023

pomegranited commented Nov 9, 2023

bradenmacdonald Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald commented Nov 9, 2023

pomegranited commented Nov 9, 2023

rpenido commented Nov 9, 2023

openedx-webhooks commented Nov 10, 2023

pomegranited commented Nov 10, 2023

rpenido commented Nov 1, 2023 •

edited

Loading

openedx-webhooks commented Nov 1, 2023 •

edited

Loading

rpenido Nov 3, 2023 •

edited

Loading

rpenido commented Nov 6, 2023 •

edited

Loading

rpenido Nov 8, 2023 •

edited

Loading

rpenido commented Nov 8, 2023 •

edited

Loading

bradenmacdonald Nov 9, 2023 •

edited

Loading