[RFC 03/11] REST: Add Projects to the API

[Andy Doan]

On 05/09/2016 08:25 AM, Finucane, Stephen wrote:

>> +++ b/patchwork/tests/test_rest_api.py

>> +    def test_list_simple(self):
>> +        """Validate we can list the default test project."""
>> +        defaults.project.save()
>> +        resp = self.client.get('/api/1.0/projects/')
>
> Q: Do we want to hardcode URLs or use 'reverse'? We don't do this
> consistently yet, but I'm moving towards it. Some users may customize
> urls.py and we should probably handle that.

I wasn't a big fan of what I did there. Version 2 uses "reverse" for this.

>> +    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(
>> +            '/api/1.0/projects/',
>> +            {'linkname': 'l', 'name': 'n', 'listid': 'l', 'listemail': 'e'})
>> +        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
>
> This is something we may wish to enable in the future, but for now
> having it disabled seems like a good move.

Yep - I just wanted to try and tests to validate all security assumptions.

>> diff --git a/patchwork/views/rest_api.py b/patchwork/views/rest_api.py

>> +class PageSizePagination(PageNumberPagination):
>> +    """Overide base class to enable the "page_size" query parameter."""
>> +    page_size = 30
>> +    page_size_query_param = 'page_size'
>
> I'm fine with 'page_size', but the spec needs to be updated if so (it
> uses 'per_page'). Alternatively, use 'per_page'?

per_page is correct, that was just typo.


>> +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 ProjectSerializer(ModelSerializer):
>> +    class Meta:
>> +        model = Project
>
> IMO, the serializers belong in their own file. This should help
> make this file more manageable as it grows.

That's fine. I'd been on the fence with this.

>> +class ProjectViewSet(ModelViewSet):
>> +    permission_classes = (PatchworkPermission,)
>> +    queryset = Project.objects.all()
>> +    serializer_class = ProjectSerializer
>> +    pagination_class = PageSizePagination
>> +
>> +
>> +router = DefaultRouter()
>> +router.register('projects', ProjectViewSet, 'project')
>>
>>   urlpatterns = [
>>       url(r'^api/1.0/', include(router.urls)),
>
> This comment belonged in the previous change, but I'm not sure about
> including urlpatterns in a file outside of 'urls.py' - it seems like
> it would be easier to debug if kept in the one location. Thoughts?

Makes sense, I'd been on the fence, but your suggestion is more 
consistent with how everything else is set up.