From 28276dfd2a2e3a23f59682422921dd28a3eb4c80 Mon Sep 17 00:00:00 2001 From: apiyo Date: Fri, 16 Dec 2022 15:07:03 +0300 Subject: [PATCH] Fix failing tests and formatting errors --- onadata/apps/api/permissions.py | 1 + .../tests/viewsets/v2/test_imports_viewset.py | 31 +++++----- onadata/apps/api/urls/v2_urls.py | 4 ++ .../apps/api/viewsets/v2/imports_viewset.py | 56 +++++++++++++------ onadata/apps/api/viewsets/xform_viewset.py | 1 - onadata/libs/tests/utils/test_async_status.py | 19 +++---- onadata/libs/utils/async_status.py | 26 ++++----- 7 files changed, 81 insertions(+), 57 deletions(-) diff --git a/onadata/apps/api/permissions.py b/onadata/apps/api/permissions.py index 01da241101..3d1bc2a119 100644 --- a/onadata/apps/api/permissions.py +++ b/onadata/apps/api/permissions.py @@ -127,6 +127,7 @@ class DjangoObjectPermissionsAllowAnon(DjangoObjectPermissions): authenticated_users_only = False +# pylint: disable=too-few-public-methods class ImportPermissions(DjangoObjectPermissions): """ ImportPermissions - custom permissions check on Imports viewset. diff --git a/onadata/apps/api/tests/viewsets/v2/test_imports_viewset.py b/onadata/apps/api/tests/viewsets/v2/test_imports_viewset.py index 52892932d2..873759a116 100644 --- a/onadata/apps/api/tests/viewsets/v2/test_imports_viewset.py +++ b/onadata/apps/api/tests/viewsets/v2/test_imports_viewset.py @@ -6,12 +6,11 @@ from typing import IO, Any from unittest.mock import patch -from django.contrib.auth.models import User +from django.contrib.auth import get_user_model from django.conf import settings from django.test import override_settings from httmock import HTTMock -from onadata.celeryapp import app from onadata.apps.api.viewsets.v2.imports_viewset import ImportsViewSet from onadata.apps.api.tests.viewsets.test_abstract_viewset import TestAbstractViewSet from onadata.apps.api.tests.mocked_data import enketo_mock @@ -29,9 +28,12 @@ def fixtures_path(filepath: str) -> IO[Any]: class TestImportsViewSet(TestAbstractViewSet): + """ + Test for ImportsViewSet + """ def setUp(self): - super(TestImportsViewSet, self).setUp() + super().setUp() self.view = ImportsViewSet.as_view({ "post": "create", "get": "retrieve", @@ -53,9 +55,9 @@ def test_create_permissions(self): "tutorial.xlsx", ) self._publish_xls_form_to_project(xlsform_path=xls_path) - user = User.objects.create(username="joe", - email="joe@example.com", - first_name="Joe") + user = get_user_model().objects.create(username="joe", + email="joe@example.com", + first_name="Joe") _ = UserProfile.objects.create(user=user) extra = {"HTTP_AUTHORIZATION": f"Token {user.auth_token}"} csv_import = fixtures_path("good.csv") @@ -102,7 +104,7 @@ def test_create_permissions(self): self.assertEqual(response.status_code, 202) @override_settings(DISABLE_ASYNCHRONOUS_IMPORTS=True) - def test_create_expected_synchronous_response(self): + def test_synchronous_response(self): """ Tests that the `api/v2/imports/` route processes a request successfully when `DISABLE_ASYNCHRONOUS_IMPORTS` is set to `True` @@ -130,7 +132,7 @@ def test_create_expected_synchronous_response(self): self.assertEqual(response.data.get("additions"), 9) self.assertEqual(response.data.get("updates"), 0) - def test_create_expected_async_response(self): + def test_expected_async_response(self): """ Tests that the `api/v2/imports/` route processes a request successfully when `DISABLE_ASYNCHRONOUS_IMPORTS` is set to `False` @@ -160,7 +162,7 @@ def test_create_expected_async_response(self): self.assertEqual(expected_fields, list(response.data.keys())) @patch("onadata.apps.api.viewsets.v2.imports_viewset.get_active_tasks") - def test_create_ongoing_overwrite_task(self, mocked_get_active_tasks): + def test_ongoing_overwrite_task(self, mocked_get_active_tasks): """ Test that the `api/v2/imports/` route refuses to process request when an overwrite import task is ongoing @@ -238,8 +240,9 @@ def test_create_request_validation(self): expected_response = {"error": "xls_file not an excel file"} self.assertEqual(expected_response, response.data) - post_data = {"csv_file": open(xls_path, "rb")} - request = self.factory.post(path, data=post_data, **self.extra) + with open(xls_path, "rb") as xls_file: + post_data = {"csv_file": xls_file} + request = self.factory.post(path, data=post_data, **self.extra) response = self.view(request, pk=self.xform.pk) self.assertEqual(response.status_code, 400) @@ -263,9 +266,9 @@ def test_delete_permissions(self): ) self._publish_xls_form_to_project(xlsform_path=xls_path) path = "/api/v2/imports/{self.xform.pk}?task_uuid=11" - user = User.objects.create(username="joe", - email="joe@example.com", - first_name="Joe") + user = get_user_model().objects.create(username="joe", + email="joe@example.com", + first_name="Joe") _ = UserProfile.objects.create(user=user) extra = {"HTTP_AUTHORIZATION": f"Token {user.auth_token}"} diff --git a/onadata/apps/api/urls/v2_urls.py b/onadata/apps/api/urls/v2_urls.py index 011deed4e4..36081a40b2 100644 --- a/onadata/apps/api/urls/v2_urls.py +++ b/onadata/apps/api/urls/v2_urls.py @@ -11,6 +11,9 @@ class DetailedPostRouter(routers.DefaultRouter): + """ + Custom router + """ routes = [ # List route. routers.Route( @@ -52,6 +55,7 @@ class DetailedPostRouter(routers.DefaultRouter): ), ] + # pylint: disable=redefined-outer-name def extend(self, router): """ Extends the routers routes with the routes from another router diff --git a/onadata/apps/api/viewsets/v2/imports_viewset.py b/onadata/apps/api/viewsets/v2/imports_viewset.py index d4c44942d5..236f55d464 100644 --- a/onadata/apps/api/viewsets/v2/imports_viewset.py +++ b/onadata/apps/api/viewsets/v2/imports_viewset.py @@ -1,4 +1,6 @@ -import json +""" +Imports v2 viewset +""" import os from django.conf import settings @@ -7,7 +9,6 @@ from rest_framework import viewsets, status from rest_framework.response import Response from rest_framework.exceptions import ParseError, ErrorDetail -from rest_framework.decorators import action from onadata.celeryapp import app from onadata.apps.api.tools import get_baseviewset_class @@ -28,28 +29,40 @@ def terminate_import_task(task_uuid: str, xform_pk: int) -> bool: + """Terminate an import task given a uuid and xform_pk""" task_details = app.control.inspect().query_task(task_uuid) if task_details: task_details = list(task_details.values()) if task_details and task_uuid in task_details[0]: task_details = task_details[0][task_uuid] - if task_details and task_details[1]["args"][1] == xform_pk: + if task_details and task_details[0] == 'active' and \ + task_details[1]["args"] and task_details[1]["args"][1] == xform_pk: app.control.terminate(task_uuid) return True return False +# pylint: disable=too-many-ancestors class ImportsViewSet(AuthenticateHeaderMixin, ETagsMixin, CacheControlMixin, viewsets.ViewSet): + """ + Implements api/v2/imports endpoints + """ permission_classes = [ImportPermissions] queryset = XForm.objects.filter(deleted_at__isnull=True) task_names = ["onadata.libs.utils.csv_import.submit_csv_async"] def get_queryset(self): + """ + Retreive all XForms that have not been deleted + """ return XForm.objects.filter(deleted_at__isnull=True) - def get_object(self, pk: int): + def get_object(self, pk: int): # pylint: disable=invalid-name + """ + Retreive XForm with specified pk + """ queryset = self.get_queryset() obj = get_object_or_404(queryset, pk=pk) self.check_object_permissions(self.request, obj) @@ -61,10 +74,12 @@ def _get_active_tasks(self, xform: XForm) -> str: """ return get_active_tasks(self.task_names, xform) + # pylint: disable=invalid-name def create(self, request, pk: int = None) -> Response: """ - Starts a new Import task for a given form; The route processes imports asynchronously - unless the `DISABLE_ASYNCHRONOUS_IMPORTS` setting is set to false. + Starts a new Import task for a given form; The route processes imports + asynchronously unless the `DISABLE_ASYNCHRONOUS_IMPORTS` setting + is set to false. Curl example: @@ -72,8 +87,9 @@ def create(self, request, pk: int = None) -> Response: Supported Query Parameters: - - [Optional] overwrite: bool = Whether the server should permanently delete the data currently available on - the form then reimport the data using the csv_file/xls_file sent with the request. + - [Optional] overwrite: bool = Whether the server should permanently + delete the data currently available on the form then reimport the + data using the csv_file/xls_file sent with the request. Required Request Arguements: @@ -82,10 +98,15 @@ def create(self, request, pk: int = None) -> Response: Possible Response status codes: - - 202 Accepted: Server has successfully accepted your request for data import and has queued the task - - 200 Ok: Server has successfully imported your data to the form; Only returned when asynchronous imports are disabled - - 400 Bad Request: Request has been refused due to incorrect/missing csv_file or xls_file file - - 403 Forbidden: The request was valid but the server refused to process it. An explanation on why it was refused can be found in the JSON Response + - 202 Accepted: Server has successfully accepted your request for data + import and has queued the task + - 200 Ok: Server has successfully imported your data to the form; + Only returned when asynchronous imports are disabled + - 400 Bad Request: Request has been refused due to incorrect/missing + csv_file or xls_file file + - 403 Forbidden: The request was valid but the server refused to + process it. An explanation on why it was refused can be found + in the JSON Response - 401 Unauthorized: The request has been refused due to missing authentication """ xform = self.get_object(pk) @@ -116,17 +137,14 @@ def create(self, request, pk: int = None) -> Response: resp.update({ "detail": ErrorDetail( - f"An ongoing overwrite request with the ID {task_id} is being processed" + "An ongoing overwrite request with the ID " + + f"{task_id} is being processed" ) }) status_code = status.HTTP_403_FORBIDDEN break if not status_code == status.HTTP_403_FORBIDDEN: - try: - csv_size = csv_file.size - except AttributeError: - csv_size = csv_file.__sizeof__() csv_file.seek(0) if getattr(settings, "DISABLE_ASYNCHRONOUS_IMPORTS", False): @@ -149,6 +167,7 @@ def create(self, request, pk: int = None) -> Response: status=status_code, content_type="application/json") + # pylint: disable=redefined-builtin, unused-argument def retrieve(self, request, pk: int = None, format=None) -> Response: """Returns csv import async tasks that belong to this form""" xform = self.get_object(pk) @@ -170,7 +189,8 @@ def destroy(self, request, pk: int = None) -> Response: Possible Response status codes: - 204 No Content: Request was successfully processed. Task was terminated. - - 400 Bad Request: Request was rejected either due to a missing `task_uuid` query parameter or because the `task_uuid` does not exist for the XForm + - 400 Bad Request: Request was rejected either due to a missing `task_uuid` + query parameter or because the `task_uuid` does not exist for the XForm """ xform = self.get_object(pk) task_uuid = request.query_params.get("task_uuid") diff --git a/onadata/apps/api/viewsets/xform_viewset.py b/onadata/apps/api/viewsets/xform_viewset.py index a918c8c29c..5c75c71281 100644 --- a/onadata/apps/api/viewsets/xform_viewset.py +++ b/onadata/apps/api/viewsets/xform_viewset.py @@ -97,7 +97,6 @@ get_form_url, ) from onadata.settings.common import CSV_EXTENSION, XLS_EXTENSIONS -from onadata.libs.utils.async_status import get_active_tasks ENKETO_AUTH_COOKIE = getattr(settings, "ENKETO_AUTH_COOKIE", "__enketo") ENKETO_META_UID_COOKIE = getattr( diff --git a/onadata/libs/tests/utils/test_async_status.py b/onadata/libs/tests/utils/test_async_status.py index 66d327a910..301867c99d 100644 --- a/onadata/libs/tests/utils/test_async_status.py +++ b/onadata/libs/tests/utils/test_async_status.py @@ -10,6 +10,7 @@ from onadata.libs.utils import async_status from onadata.apps.logger.models.xform import XForm + class TestAsyncStatus(TestBase): def test_celery_state_to_status(self): @@ -50,7 +51,7 @@ def test_get_active_tasks(self): async_status.get_active_tasks( ['onadata.libs.utils.csv_import.submit_csv_async'], xform ), - '[]', + [], ) inspect = MagicMock() inspect.active = MagicMock( @@ -67,12 +68,10 @@ def test_get_active_tasks(self): ) app.control.inspect = MagicMock(return_value=inspect) - self.assertEqual( - async_status.get_active_tasks( - ['onadata.libs.utils.csv_import.submit_csv_async'], xform - ), - '[{"job_uuid": "11", "time_start"' - + ": \"" - + datetime.fromtimestamp(time_start).strftime("%Y-%m-%dT%H:%M:%S") - + '", "file": "/home/ona/import.csv", "overwrite": true}]', - ) + self.assertEqual(async_status.get_active_tasks( + ['onadata.libs.utils.csv_import.submit_csv_async'], + xform), + [{'job_uuid': '11', + 'time_start': datetime.fromtimestamp(time_start).strftime( + "%Y-%m-%dT%H:%M:%S"), + "file": "/home/ona/import.csv", "overwrite": True}]) diff --git a/onadata/libs/utils/async_status.py b/onadata/libs/utils/async_status.py index 79414eee00..1ced064621 100644 --- a/onadata/libs/utils/async_status.py +++ b/onadata/libs/utils/async_status.py @@ -1,7 +1,6 @@ """ Utilities for celery asyncronous tasks """ -import json from datetime import datetime from typing import List @@ -47,6 +46,17 @@ def async_status(status, error=None): return status +def format_task(task): + """Format a celery task element""" + return {"job_uuid": gettext(task["id"]), + "time_start": datetime.fromtimestamp(task["time_start"]).strftime( + "%Y-%m-%dT%H:%M:%S" + ), + "file": gettext(task["args"][2]), + "overwrite": task["args"][3], + } + + def get_active_tasks(task_names: List[str], xform: XForm): """Get active celery tasks""" inspect = app.control.inspect() @@ -61,16 +71,4 @@ def get_active_tasks(task_names: List[str], xform: XForm): ) ) - return list( - map( - lambda i: { - "job_uuid": gettext(i["id"]), - "time_start": datetime.fromtimestamp(i["time_start"]).strftime( - "%Y-%m-%dT%H:%M:%S" - ), - "file": gettext(i["args"][2]), - "overwrite": i["args"][3], - }, - data, - ) - ) \ No newline at end of file + return list(map(format_task, data))