diff mbox

[v6,07/10] REST: Add Patch Checks to the API

Message ID 1466111605-3059-8-git-send-email-andy.doan@linaro.org
State Accepted
Headers show

Commit Message

Andy Doan June 16, 2016, 9:13 p.m. UTC
This exports patch checks via the REST API.

The drf-nested-routers package is used to handle the fact Checks are
nested under a Patch.

Security Constraints:
 * Anyone (logged in or not) can read all objects.
 * No one can update/delete objects.
 * Project maintainers and patch owners may create objects.

Signed-off-by: Andy Doan <andy.doan@linaro.org>
---
 patchwork/rest_serializers.py    | 42 +++++++++++++++++-
 patchwork/tests/test_rest_api.py | 94 +++++++++++++++++++++++++++++++++++++++-
 patchwork/urls.py                |  6 +--
 patchwork/views/rest_api.py      | 40 ++++++++++++++++-
 requirements-test.txt            |  1 +
 5 files changed, 175 insertions(+), 8 deletions(-)

Comments

Stephen Finucane June 27, 2016, 5:06 p.m. UTC | #1
On 16 Jun 16:13, Andy Doan wrote:
> This exports patch checks via the REST API.
> 
> The drf-nested-routers package is used to handle the fact Checks are
> nested under a Patch.
> 
> Security Constraints:
>  * Anyone (logged in or not) can read all objects.
>  * No one can update/delete objects.
>  * Project maintainers and patch owners may create objects.
> 
> Signed-off-by: Andy Doan <andy.doan@linaro.org>

I made some changes to this to make the 'date' field read-only (think
there was some miscommunication here). After this:

Reviewed-by: Stephen Finucane <stephen.finucane@intel.com>

> +class ChecksSerializer(ModelSerializer):
> +    class Meta:
> +        model = Check

I added:

    read_only_fields = ('date', )

> +    user = HyperlinkedRelatedField(
> +        'user-detail', read_only=True, default=CurrentUserDefault())
> +    patch = HiddenField(default=CurrentPatchDefault())
> +    date = DateTimeField(required=True)

...and I removed this ^^^.
diff mbox

Patch

diff --git a/patchwork/rest_serializers.py b/patchwork/rest_serializers.py
index 12a7199..bfeaca8 100644
--- a/patchwork/rest_serializers.py
+++ b/patchwork/rest_serializers.py
@@ -20,12 +20,14 @@ 
 import email.parser
 
 from django.contrib.auth.models import User
+from django.core.urlresolvers import reverse
 
 from rest_framework.relations import HyperlinkedRelatedField
 from rest_framework.serializers import (
-    HyperlinkedModelSerializer, ListSerializer, SerializerMethodField)
+    CurrentUserDefault, DateTimeField, HiddenField, HyperlinkedModelSerializer,
+    ListSerializer, ModelSerializer, SerializerMethodField)
 
-from patchwork.models import Patch, Person, Project
+from patchwork.models import Check, Patch, Person, Project
 
 
 class URLSerializer(HyperlinkedModelSerializer):
@@ -90,9 +92,45 @@  class PatchSerializer(URLSerializer):
 
     def to_representation(self, instance):
         data = super(PatchSerializer, self).to_representation(instance)
+        data['checks_url'] = data['url'] + 'checks/'
         headers = data.get('headers')
         if headers is not None:
             data['headers'] = email.parser.Parser().parsestr(headers, True)
         data['tags'] = [{'name': x.tag.name, 'count': x.count}
                         for x in instance.patchtag_set.all()]
         return data
+
+
+class CurrentPatchDefault(object):
+    def set_context(self, serializer_field):
+        self.patch = serializer_field.context['request'].patch
+
+    def __call__(self):
+        return self.patch
+
+
+class ChecksSerializer(ModelSerializer):
+    class Meta:
+        model = Check
+    user = HyperlinkedRelatedField(
+        'user-detail', read_only=True, default=CurrentUserDefault())
+    patch = HiddenField(default=CurrentPatchDefault())
+    date = DateTimeField(required=True)
+
+    def run_validation(self, data):
+        for val, label in Check.STATE_CHOICES:
+            if label == data['state']:
+                data['state'] = val
+                break
+        return super(ChecksSerializer, self).run_validation(data)
+
+    def to_representation(self, instance):
+        data = super(ChecksSerializer, self).to_representation(instance)
+        data['state'] = instance.get_state_display()
+        # drf-nested doesn't handle HyperlinkedModelSerializers properly,
+        # so we have to put the url in by hand here.
+        url = self.context['request'].build_absolute_uri(reverse(
+            'api_1.0:patch-detail', args=[instance.patch.id]))
+        data['url'] = url + 'checks/%s/' % instance.id
+        data['users_url'] = data.pop('user')
+        return data
diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py
index c30e90e..3ed8469 100644
--- a/patchwork/tests/test_rest_api.py
+++ b/patchwork/tests/test_rest_api.py
@@ -25,7 +25,7 @@  from django.core.urlresolvers import reverse
 from rest_framework import status
 from rest_framework.test import APITestCase
 
-from patchwork.models import Patch, Project
+from patchwork.models import Check, Patch, Project
 from patchwork.tests.utils import (
     defaults, create_maintainer, create_user, create_patches, make_msgid)
 
@@ -338,3 +338,95 @@  class TestPatchAPI(APITestCase):
         resp = self.client.delete(self.api_url(patches[0].id))
         self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
         self.assertEqual(1, Patch.objects.all().count())
+
+
+@unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
+class TestCheckAPI(APITestCase):
+    fixtures = ['default_states', 'default_tags']
+
+    def setUp(self):
+        super(TestCheckAPI, self).setUp()
+        self.patch = create_patches()[0]
+        self.urlbase = reverse('api_1.0:patch-detail', args=[self.patch.id])
+        self.urlbase += 'checks/'
+        defaults.project.save()
+        self.user = create_maintainer(defaults.project)
+
+    def create_check(self):
+        return Check.objects.create(patch=self.patch, user=self.user,
+                                    state=Check.STATE_WARNING, target_url='t',
+                                    description='d', context='c')
+
+    def test_list_simple(self):
+        """Validate we can list checks on a patch."""
+        resp = self.client.get(self.urlbase)
+        self.assertEqual(status.HTTP_200_OK, resp.status_code)
+        self.assertEqual(0, len(resp.data))
+
+        c = self.create_check()
+        resp = self.client.get(self.urlbase)
+        self.assertEqual(status.HTTP_200_OK, resp.status_code)
+        self.assertEqual(1, len(resp.data))
+        check = resp.data[0]
+        self.assertEqual(c.get_state_display(), check['state'])
+        self.assertEqual(c.target_url, check['target_url'])
+        self.assertEqual(c.context, check['context'])
+        self.assertEqual(c.description, check['description'])
+
+    def test_detail(self):
+        """Validate we can get a specific check."""
+        c = self.create_check()
+        resp = self.client.get(self.urlbase + str(c.id) + '/')
+        self.assertEqual(status.HTTP_200_OK, resp.status_code)
+        self.assertEqual(c.target_url, resp.data['target_url'])
+
+    def test_update_delete(self):
+        """Ensure updates and deletes aren't allowed"""
+        c = self.create_check()
+
+        self.user.is_superuser = True
+        self.user.save()
+        self.client.force_authenticate(user=self.user)
+
+        # update
+        resp = self.client.patch(
+            self.urlbase + str(c.id) + '/', {'target_url': 'fail'})
+        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
+        # delete
+        resp = self.client.delete(self.urlbase + str(c.id) + '/')
+        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
+
+    def test_create(self):
+        """Ensure creations can be performed by user of patch."""
+        check = {
+            'state': 'success',
+            'target_url': 'http://t.co',
+            'description': 'description',
+            'context': 'context',
+            'date': '2016-02-18T20:39:01.858734',
+        }
+
+        self.client.force_authenticate(user=self.user)
+        resp = self.client.post(self.urlbase, check)
+        self.assertEqual(status.HTTP_201_CREATED, resp.status_code)
+        self.assertEqual(1, Check.objects.all().count())
+
+        user = create_user()
+        self.client.force_authenticate(user=user)
+        resp = self.client.post(self.urlbase, check)
+        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
+
+    def test_create_invalid(self):
+        """Ensure we handle invalid check states"""
+        check = {
+            'state': 'this-is-not-a-valid-state',
+            'target_url': 'http://t.co',
+            'description': 'description',
+            'context': 'context',
+            'date': '2016-02-18T20:39:01.858734',
+        }
+
+        self.client.force_authenticate(user=self.user)
+        resp = self.client.post(self.urlbase, check)
+        self.assertEqual(status.HTTP_400_BAD_REQUEST, resp.status_code)
+        self.assertEqual(0, Check.objects.all().count())
diff --git a/patchwork/urls.py b/patchwork/urls.py
index f6763d4..c97b422 100644
--- a/patchwork/urls.py
+++ b/patchwork/urls.py
@@ -142,10 +142,10 @@  if settings.ENABLE_REST_API:
     if 'rest_framework' not in settings.INSTALLED_APPS:
         raise RuntimeError(
             'djangorestframework must be installed to enable the REST API.')
-    import patchwork.views.rest_api
+    from patchwork.views.rest_api import router, patches_router
     urlpatterns += [
-        url(r'^api/1.0/', include(
-            patchwork.views.rest_api.router.urls, namespace='api_1.0')),
+        url(r'^api/1.0/', include(router.urls, namespace='api_1.0')),
+        url(r'^api/1.0/', include(patches_router.urls, namespace='api_1.0')),
     ]
 
 # redirect from old urls
diff --git a/patchwork/views/rest_api.py b/patchwork/views/rest_api.py
index 2ab0f97..7d798a1 100644
--- a/patchwork/views/rest_api.py
+++ b/patchwork/views/rest_api.py
@@ -18,15 +18,18 @@ 
 # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 
 from django.conf import settings
-
+from patchwork.models import Patch
 from patchwork.rest_serializers import (
-    PatchSerializer, PersonSerializer, ProjectSerializer, UserSerializer)
+    ChecksSerializer, PatchSerializer, PersonSerializer, ProjectSerializer,
+    UserSerializer)
 
 from rest_framework import permissions
+from rest_framework.exceptions import PermissionDenied
 from rest_framework.pagination import PageNumberPagination
 from rest_framework.response import Response
 from rest_framework.routers import DefaultRouter
 from rest_framework.viewsets import ModelViewSet
+from rest_framework_nested.routers import NestedSimpleRouter
 
 
 class LinkHeaderPagination(PageNumberPagination):
@@ -114,8 +117,41 @@  class PatchViewSet(PatchworkViewSet):
         return qs
 
 
+class CheckViewSet(PatchworkViewSet):
+    serializer_class = ChecksSerializer
+
+    def not_allowed(self, request, **kwargs):
+        raise PermissionDenied()
+
+    update = not_allowed
+    partial_update = not_allowed
+    destroy = not_allowed
+
+    def create(self, request, patch_pk):
+        p = Patch.objects.get(id=patch_pk)
+        if not p.is_editable(request.user):
+            raise PermissionDenied()
+        request.patch = p
+        return super(CheckViewSet, self).create(request)
+
+    def list(self, request, patch_pk):
+        queryset = self.filter_queryset(self.get_queryset())
+        queryset = queryset.filter(patch=patch_pk)
+
+        page = self.paginate_queryset(queryset)
+        if page is not None:
+            serializer = self.get_serializer(page, many=True)
+            return self.get_paginated_response(serializer.data)
+
+        serializer = self.get_serializer(queryset, many=True)
+        return Response(serializer.data)
+
+
 router = DefaultRouter()
 router.register('patches', PatchViewSet, 'patch')
 router.register('people', PeopleViewSet, 'person')
 router.register('projects', ProjectViewSet, 'project')
 router.register('users', UserViewSet, 'user')
+
+patches_router = NestedSimpleRouter(router, r'patches', lookup='patch')
+patches_router.register(r'checks', CheckViewSet, base_name='patch-checks')
diff --git a/requirements-test.txt b/requirements-test.txt
index b5f976c..cfc242f 100644
--- a/requirements-test.txt
+++ b/requirements-test.txt
@@ -3,3 +3,4 @@  django-debug-toolbar==1.4
 python-dateutil>2.0,<3.0
 selenium>2.0,<3.0
 djangorestframework>=3.3,<3.4
+drf-nested-routers>=0.11.1,<0.12