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

Add endpoint for fetching currently running csv imports #2299

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

FrankApiyo
Copy link
Member

@FrankApiyo FrankApiyo commented Aug 18, 2022

Before submitting this PR for review, please make sure you have:

  • Included tests
  • Updated documentation

Closes #2204

@FrankApiyo FrankApiyo force-pushed the add-mechanism-to-get-ongoing-csv-imports branch 5 times, most recently from 3c79a16 to 863f4e8 Compare September 27, 2022 13:52
@FrankApiyo FrankApiyo changed the title [WIP]Add endpoint for fetching currently running csv imports Add endpoint for fetching currently running csv imports Sep 28, 2022
DavisRayM
DavisRayM previously approved these changes Sep 29, 2022
@FrankApiyo FrankApiyo force-pushed the add-mechanism-to-get-ongoing-csv-imports branch 3 times, most recently from 036b4ab to 26080a3 Compare September 29, 2022 13:25
KipSigei
KipSigei previously approved these changes Sep 29, 2022
@FrankApiyo FrankApiyo force-pushed the add-mechanism-to-get-ongoing-csv-imports branch from 3293457 to f55cbf3 Compare December 16, 2022 09:58
@FrankApiyo FrankApiyo force-pushed the add-mechanism-to-get-ongoing-csv-imports branch 8 times, most recently from 28276df to cc30a35 Compare December 16, 2022 14:59
@FrankApiyo FrankApiyo force-pushed the add-mechanism-to-get-ongoing-csv-imports branch from cc30a35 to d54af21 Compare December 19, 2022 06:44
overwrite, str) else overwrite)

# Block imports from running when an overwrite is ongoing
active_tasks = self._get_active_tasks(xform)
Copy link
Member

Choose a reason for hiding this comment

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

Why not get_active_tasks(self.task_names, xform) it is still clean and there is nothing special about the _get_active_tasks() that requires it to be a class method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will make this change

Comment on lines +13 to +71
class DetailedPostRouter(routers.DefaultRouter):
"""
Custom router
"""
routes = [
# List route.
routers.Route(
url=r"^{prefix}{trailing_slash}$",
mapping={"get": "list", "post": "create"},
name="{basename}-list",
detail=False,
initkwargs={"suffix": "List"},
),
# Dynamically generated list routes. Generated using
# @action(detail=False) decorator on methods of the viewset.
routers.DynamicRoute(
url=r"^{prefix}/{url_path}{trailing_slash}$",
name="{basename}-{url_name}",
detail=False,
initkwargs={},
),
# Detail route.
routers.Route(
url=r"^{prefix}/{lookup}{trailing_slash}$",
mapping={
"get": "retrieve",
"put": "update",
"patch": "partial_update",
"delete": "destroy",
"post": "create",
},
name="{basename}-detail",
detail=True,
initkwargs={"suffix": "Instance"},
),
# Dynamically generated detail routes. Generated using
# @action(detail=True) decorator on methods of the viewset.
routers.DynamicRoute(
url=r"^{prefix}/{lookup}/{url_path}{trailing_slash}$",
name="{basename}-{url_name}",
detail=True,
initkwargs={},
),
]

# pylint: disable=redefined-outer-name
def extend(self, router):
"""
Extends the routers routes with the routes from another router
"""
self.registry.extend(router.registry)


base_router = MultiLookupRouter(trailing_slash=False)
router = DetailedPostRouter(trailing_slash=False)
base_router.register(r"open-data", TableauViewSet, basename="open-data")
router.register(r"imports", ImportsViewSet, basename="imports")

router.extend(base_router)
Copy link
Member

Choose a reason for hiding this comment

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

Why is the DetailedPostRouter necessary? Which path is impossible to do with the existing default and custom routers that we need another custom router?

Copy link
Contributor

Choose a reason for hiding this comment

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

This was mostly implemented so that we could pass a detail to a POST route; AFAIK the post (create) route on DRF doesn't usually support detail... Is there a particular way we could implement this without having to create our own custom route?

tl;dr This was done so that we could pass detail to the default create function; Allowing requests such as curl -X POST "http://<domain>/api/v2/imports/<xform_id>"

Copy link
Member

Choose a reason for hiding this comment

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

We should use create/post on the /api/v2/imports to create a new import instead of /api/v2/imports/<xform_id>. The xform_id should be a mandatory post parameter in the post to create an import. With that approach, there is no need for a custom router. We already take this approach with the /api/v1/forms, /api/v1/dataviews and other endpoints.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a mechanism by which we can know when a csv import task is being carried on on a form
4 participants