[PATCHv2,03/10] REST: Add Projects to the API

This exports projects via the REST API.

Security Constraints:
 * Anyone (logged in or not) can read all objects.
 * No one can create/delete objects.
 * Project maintainers are allowed to update (ie "patch"
   attributes)

Signed-off-by: Andy Doan <andy.doan@linaro.org>
Inspired-by: Damien Lespiau <damien.lespiau@intel.com>
---
 patchwork/rest_serializers.py    |  27 +++++++++
 patchwork/tests/test_rest_api.py | 115 +++++++++++++++++++++++++++++++++++++++
 patchwork/urls.py                |   3 +-
 patchwork/views/rest_api.py      |  39 ++++++++++++-
 4 files changed, 181 insertions(+), 3 deletions(-)
 create mode 100644 patchwork/rest_serializers.py
 create mode 100644 patchwork/tests/test_rest_api.py

On 10 May 17:39, Andy Doan wrote:
> This exports projects via the REST API.
> 
> Security Constraints:
>  * Anyone (logged in or not) can read all objects.
>  * No one can create/delete objects.
>  * Project maintainers are allowed to update (ie "patch"
>    attributes)
> 
> Signed-off-by: Andy Doan <andy.doan@linaro.org>
> Inspired-by: Damien Lespiau <damien.lespiau@intel.com>

Looks good. Some nits, but nothing I would block on. Maybe fix them in
a v3 (for the 'check' field in 'patch' response).

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

> ---
>  patchwork/rest_serializers.py    |  27 +++++++++
>  patchwork/tests/test_rest_api.py | 115 +++++++++++++++++++++++++++++++++++++++
>  patchwork/urls.py                |   3 +-
>  patchwork/views/rest_api.py      |  39 ++++++++++++-
>  4 files changed, 181 insertions(+), 3 deletions(-)
>  create mode 100644 patchwork/rest_serializers.py
>  create mode 100644 patchwork/tests/test_rest_api.py
> 
> diff --git a/patchwork/rest_serializers.py b/patchwork/rest_serializers.py
> new file mode 100644
> index 0000000..b399d79
> --- /dev/null
> +++ b/patchwork/rest_serializers.py
> @@ -0,0 +1,27 @@
> +# Patchwork - automated patch tracking system
> +# Copyright (C) 2016 Linaro Corporation
> +#
> +# This file is part of the Patchwork package.
> +#
> +# Patchwork is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# Patchwork is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with Patchwork; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> +
> +from patchwork.models import Project
> +
> +from rest_framework.serializers import ModelSerializer
> +
> +
> +class ProjectSerializer(ModelSerializer):
> +    class Meta:
> +        model = Project
> diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py
> new file mode 100644
> index 0000000..693e896
> --- /dev/null
> +++ b/patchwork/tests/test_rest_api.py
> @@ -0,0 +1,115 @@
> +# Patchwork - automated patch tracking system
> +# Copyright (C) 2016 Linaro Corporation
> +#
> +# This file is part of the Patchwork package.
> +#
> +# Patchwork is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# Patchwork is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with Patchwork; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> +
> +import unittest
> +
> +from django.conf import settings
> +from django.core.urlresolvers import reverse
> +
> +from rest_framework import status
> +from rest_framework.test import APITestCase
> +
> +from patchwork.models import Project
> +from patchwork.tests.utils import defaults, create_maintainer, create_user
> +
> +
> +@unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
> +class TestProjectAPI(APITestCase):
> +    fixtures = ['default_states']
> +    apibase = reverse('api_1.0:project-list')
> +
> +    def api_url(self, item=None):
> +        if item is None:
> +            return reverse('api_1.0:project-list')
> +        return reverse('api_1.0:project-detail', args=[item])

nit: This should be a classmethod or separate function (no use of
self).

> +
> +    def test_list_simple(self):
> +        """Validate we can list the default test project."""
> +        defaults.project.save()
> +        resp = self.client.get(self.api_url())
> +        self.assertEqual(status.HTTP_200_OK, resp.status_code)
> +        self.assertEqual(1, resp.data['count'])
> +        proj = resp.data['results'][0]
> +        self.assertEqual(defaults.project.linkname, proj['linkname'])
> +        self.assertEqual(defaults.project.name, proj['name'])
> +        self.assertEqual(defaults.project.listid, proj['listid'])
> +
> +    def test_get(self):
> +        """Validate we can get a specific project."""
> +        defaults.project.save()
> +        resp = self.client.get(self.api_url(1))
> +        self.assertEqual(status.HTTP_200_OK, resp.status_code)
> +        self.assertEqual(defaults.project.name, resp.data['name'])
> +
> +    def test_anonymous_writes(self):
> +        """Ensure anonymous "write" operations are rejected."""
> +        defaults.project.save()
> +        # create
> +        resp = self.client.post(
> +            self.api_url(),
> +            {'linkname': 'l', 'name': 'n', 'listid': 'l', 'listemail': 'e'})
> +        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +        # update
> +        resp = self.client.patch(self.api_url(1), {'linkname': 'foo'})
> +        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +        # delete
> +        resp = self.client.delete(self.api_url(1))
> +        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +
> +    def test_create(self):
> +        """Ensure creations are rejected."""
> +        defaults.project.save()
> +
> +        user = create_maintainer(defaults.project)
> +        user.is_superuser = True
> +        user.save()
> +        self.client.force_authenticate(user=user)
> +        resp = self.client.post(
> +            self.api_url(),
> +            {'linkname': 'l', 'name': 'n', 'listid': 'l', 'listemail': 'e'})
> +        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +
> +    def test_update(self):
> +        """Ensure updates can be performed maintainers."""
> +        defaults.project.save()
> +
> +        # A maintainer can update
> +        user = create_maintainer(defaults.project)
> +        self.client.force_authenticate(user=user)
> +        resp = self.client.patch(self.api_url(1), {'linkname': 'TEST'})
> +        self.assertEqual(status.HTTP_200_OK, resp.status_code)
> +
> +        # A normal user can't
> +        user = create_user()
> +        self.client.force_authenticate(user=user)
> +        resp = self.client.patch(self.api_url(1), {'linkname': 'TEST'})
> +        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +
> +    def test_delete(self):
> +        """Ensure deletions are rejected."""
> +        defaults.project.save()
> +
> +        # Even an admin can't remove a project
> +        user = create_maintainer(defaults.project)
> +        user.is_superuser = True
> +        user.save()
> +        self.client.force_authenticate(user=user)
> +        resp = self.client.delete(self.api_url(1))
> +        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +        self.assertEqual(1, Project.objects.all().count())
> diff --git a/patchwork/urls.py b/patchwork/urls.py
> index 3f0fcbf..f6763d4 100644
> --- a/patchwork/urls.py
> +++ b/patchwork/urls.py
> @@ -144,7 +144,8 @@ if settings.ENABLE_REST_API:
>              'djangorestframework must be installed to enable the REST API.')
>      import patchwork.views.rest_api
>      urlpatterns += [
> -        url(r'^api/1.0/', include(patchwork.views.rest_api.router.urls)),
> +        url(r'^api/1.0/', include(
> +            patchwork.views.rest_api.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 5436ed6..33fd63b 100644
> --- a/patchwork/views/rest_api.py
> +++ b/patchwork/views/rest_api.py
> @@ -17,6 +17,41 @@
>  # along with Patchwork; if not, write to the Free Software
>  # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>  
> -from rest_framework import routers
> +from patchwork.models import Project
> +from patchwork.rest_serializers import ProjectSerializer
>  
> -router = routers.DefaultRouter()
> +from rest_framework import permissions
> +from rest_framework.pagination import PageNumberPagination
> +from rest_framework.routers import DefaultRouter
> +from rest_framework.viewsets import ModelViewSet
> +
> +
> +class PageSizePagination(PageNumberPagination):
> +    """Overide base class to enable the "page_size" query parameter."""

s/page_size/per_page/?

> +    page_size = 30

Maybe this config could be centralized in 'settings.py'?

> +    page_size_query_param = 'per_page'
> +
> +
> +class PatchworkPermission(permissions.BasePermission):
> +    """This permission works for Project and Patch model objects"""
> +    def has_permission(self, request, view):
> +        if request.method in ('POST', 'DELETE'):
> +            return False
> +        return super(PatchworkPermission, self).has_permission(request, view)
> +
> +    def has_object_permission(self, request, view, obj):
> +        # read only for everyone
> +        if request.method in permissions.SAFE_METHODS:
> +            return True
> +        return obj.is_editable(request.user)
> +
> +
> +class ProjectViewSet(ModelViewSet):
> +    permission_classes = (PatchworkPermission,)

nit: space after comma (maybe run this through 'tox -e pep8' - it
should be clean right now...)

> +    queryset = Project.objects.all()
> +    serializer_class = ProjectSerializer
> +    pagination_class = PageSizePagination
> +
> +
> +router = DefaultRouter()
> +router.register('projects', ProjectViewSet, 'project')
> -- 
> 2.7.4
> 
> _______________________________________________
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork

On 05/17/2016 07:25 AM, Finucane, Stephen wrote:
>> +class ProjectViewSet(ModelViewSet):
>> >+    permission_classes = (PatchworkPermission,)
> nit: space after comma (maybe run this through 'tox -e pep8' - it
> should be clean right now...)

That's not showing up under my version of flake8 or pyflakes, but I've 
updated it to keep consistency with you