Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 #112
feat: add import taxonomy endpoint #112
Changes from 2 commits
c1d5944
05105bc
a6919a1
f030767
4fe2457
06efd55
d400a54
a50e08f
e1b1789
d9950f6
7450737
ae76400
62e2593
a827178
730b73f
4e52c57
a8a0c7e
c4fe95d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we mean to make the call async here?
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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."