On Wed, 2019-10-02 at 11:50 +1000, Daniel Axtens wrote: > Stephen Finucane <step...@that.guru> writes: > > > Allow users to create a new bundle, change the name, public flag and > > patches of an existing bundle, and delete an existing bundle. > > > > Some small nits with existing tests are resolved. > > > > Signed-off-by: Stephen Finucane <step...@that.guru> > > --- > > docs/api/schemas/latest/patchwork.yaml | 170 +++++++++++++++- > > docs/api/schemas/patchwork.j2 | 181 +++++++++++++++++- > > docs/api/schemas/v1.0/patchwork.yaml | 5 +- > > docs/api/schemas/v1.1/patchwork.yaml | 5 +- > > docs/api/schemas/v1.2/patchwork.yaml | 170 +++++++++++++++- > > patchwork/api/bundle.py | 84 +++++++- > > patchwork/models.py | 11 ++ > > patchwork/tests/api/test_bundle.py | 118 +++++++++++- > > patchwork/tests/api/utils.py | 16 +- > > ...pdate-bundle-via-api-2946d8c4e730d545.yaml | 4 + > > 10 files changed, 737 insertions(+), 27 deletions(-) > > create mode 100644 > > releasenotes/notes/update-bundle-via-api-2946d8c4e730d545.yaml > > > > diff --git a/docs/api/schemas/latest/patchwork.yaml > > b/docs/api/schemas/latest/patchwork.yaml > > index 45a61180..e6c6bb4a 100644 > > --- a/docs/api/schemas/latest/patchwork.yaml > > +++ b/docs/api/schemas/latest/patchwork.yaml
[snip] > > diff --git a/patchwork/api/bundle.py b/patchwork/api/bundle.py > > index 2dec70d1..c5885aae 100644 > > --- a/patchwork/api/bundle.py > > +++ b/patchwork/api/bundle.py > > @@ -4,9 +4,12 @@ > > # SPDX-License-Identifier: GPL-2.0-or-later > > > > from django.db.models import Q > > -from rest_framework.generics import ListAPIView > > -from rest_framework.generics import RetrieveAPIView > > +from rest_framework import exceptions > > +from rest_framework.generics import ListCreateAPIView > > +from rest_framework.generics import RetrieveUpdateDestroyAPIView > > +from rest_framework import permissions > > from rest_framework.serializers import SerializerMethodField > > +from rest_framework.serializers import ValidationError > > > > from patchwork.api.base import BaseHyperlinkedModelSerializer > > from patchwork.api.base import PatchworkPermission > > @@ -14,16 +17,52 @@ from patchwork.api.filters import BundleFilterSet > > from patchwork.api.embedded import PatchSerializer > > from patchwork.api.embedded import ProjectSerializer > > from patchwork.api.embedded import UserSerializer > > +from patchwork.api import utils > > from patchwork.models import Bundle > > > > > > +class BundlePermission(permissions.BasePermission): > > + """Ensure the API version, if configured, is >= v1.2. > > + > > + Bundle creation/updating was only added in API v1.2 and we don't want > > to > > + change behavior in older API versions. > > + """ > > + def has_permission(self, request, view): > > + # read-only permission for everything > > + if request.method in permissions.SAFE_METHODS: > > + return True > > + > > + if not utils.has_version(request, '1.2'): > > + raise exceptions.MethodNotAllowed(request.method) > > + > > + if request.method == 'POST' and ( > > + not request.user or not request.user.is_authenticated): > > + return False > > + > > + # we have more to do but we can't do that until we have an object > > + return True > > + > > + def has_object_permission(self, request, view, obj): > > + if (request.user and > > + request.user.is_authenticated and > > + request.user == obj.owner): > > Just checking that this doesn't have the same User vs UserProfile thing > we had with delegation? I haven't checked in great detail, I just saw a > user comparison and thought it best to ask! No, we're good here since Bundle.user points to the User object, not the UserProfile object. > > + return True > > + > > + if not obj.public: > > + # if the bundle isn't public, we don't want to leak the fact > > that > > + # it exists > > + raise exceptions.NotFound > > A++ > > > + > > + return request.method in permissions.SAFE_METHODS > > + > > + > > class BundleSerializer(BaseHyperlinkedModelSerializer): > > > > web_url = SerializerMethodField() > > project = ProjectSerializer(read_only=True) > > mbox = SerializerMethodField() > > owner = UserSerializer(read_only=True) > > - patches = PatchSerializer(many=True, read_only=True) > > + patches = PatchSerializer(many=True) > > > > def get_web_url(self, instance): > > request = self.context.get('request') > > @@ -33,11 +72,35 @@ class BundleSerializer(BaseHyperlinkedModelSerializer): > > request = self.context.get('request') > > return request.build_absolute_uri(instance.get_mbox_url()) > > > > + def create(self, validated_data): > > + patches = validated_data.pop('patches') > > + instance = super(BundleSerializer, self).create(validated_data) > > + instance.overwrite_patches(patches) > > + return instance > > + > > + def update(self, instance, validated_data): > > + patches = validated_data.pop('patches') > > + instance = super(BundleSerializer, self).update( > > + instance, validated_data) > > + instance.overwrite_patches(patches) > > + return instance > > + > > + def validate(self, data): > > + if not data.get('patches'): > > + raise ValidationError('Bundles cannot be empty') > > + > > + if len(set([p.project.id for p in data['patches']])) > 1: > > + raise ValidationError('Patches must belong to the same > > project') > > + > > + data['project'] = data['patches'][0].project > > + > > + return super(BundleSerializer, self).validate(data) > > + > > class Meta: > > model = Bundle > > fields = ('id', 'url', 'web_url', 'project', 'name', 'owner', > > 'patches', 'public', 'mbox') > > - read_only_fields = ('owner', 'patches', 'mbox') > > + read_only_fields = ('project', 'owner', 'mbox') > > versioned_fields = { > > '1.1': ('web_url', ), > > } > > @@ -48,7 +111,7 @@ class BundleSerializer(BaseHyperlinkedModelSerializer): > > > > class BundleMixin(object): > > > > - permission_classes = (PatchworkPermission,) > > + permission_classes = [PatchworkPermission & BundlePermission] > > serializer_class = BundleSerializer > > > > def get_queryset(self): > > @@ -63,16 +126,19 @@ class BundleMixin(object): > > .select_related('owner', 'project') > > > > > > -class BundleList(BundleMixin, ListAPIView): > > - """List bundles.""" > > +class BundleList(BundleMixin, ListCreateAPIView): > > + """List or create bundles.""" > > > > filter_class = filterset_class = BundleFilterSet > > search_fields = ('name',) > > ordering_fields = ('id', 'name', 'owner') > > ordering = 'id' > > > > + def perform_create(self, serializer): > > + serializer.save(owner=self.request.user) > > + > Likewise and throughout the patch w/ user types. > > Apart from that, I think this is absolutely a Good Idea and I hope to do > some testing of it soon. > > Regards, > Daniel > > > > > -class BundleDetail(BundleMixin, RetrieveAPIView): > > - """Show a bundle.""" > > +class BundleDetail(BundleMixin, RetrieveUpdateDestroyAPIView): > > + """Show, update or delete a bundle.""" > > > > pass > > diff --git a/patchwork/models.py b/patchwork/models.py > > index 32d1b3c2..631de85d 100644 > > --- a/patchwork/models.py > > +++ b/patchwork/models.py > > @@ -788,6 +788,11 @@ class Bundle(models.Model): > > patches = models.ManyToManyField(Patch, through='BundlePatch') > > public = models.BooleanField(default=False) > > > > + def is_editable(self, user): > > + if not user.is_authenticated: > > + return False > > + return user == self.owner > > + > > def ordered_patches(self): > > return self.patches.order_by('bundlepatch__order') > > > > @@ -806,6 +811,12 @@ class Bundle(models.Model): > > return BundlePatch.objects.create(bundle=self, patch=patch, > > order=max_order + 1) > > > > + def overwrite_patches(self, patches): > > + BundlePatch.objects.filter(bundle=self).delete() > > + > > + for patch in patches: > > + self.append_patch(patch) > > + > > def get_absolute_url(self): > > return reverse('bundle-detail', kwargs={ > > 'username': self.owner.username, > > diff --git a/patchwork/tests/api/test_bundle.py > > b/patchwork/tests/api/test_bundle.py > > index 303c500c..a3a0c113 100644 > > --- a/patchwork/tests/api/test_bundle.py > > +++ b/patchwork/tests/api/test_bundle.py > > @@ -8,9 +8,11 @@ import unittest > > from django.conf import settings > > from django.urls import reverse > > > > +from patchwork.models import Bundle > > from patchwork.tests.api import utils > > from patchwork.tests.utils import create_bundle > > from patchwork.tests.utils import create_maintainer > > +from patchwork.tests.utils import create_patch > > from patchwork.tests.utils import create_project > > from patchwork.tests.utils import create_user > > > > @@ -42,12 +44,15 @@ class TestBundleAPI(utils.APITestCase): > > > > # nested fields > > > > - self.assertEqual(bundle_obj.patches.count(), > > - len(bundle_json['patches'])) > > self.assertEqual(bundle_obj.owner.id, > > bundle_json['owner']['id']) > > self.assertEqual(bundle_obj.project.id, > > bundle_json['project']['id']) > > + self.assertEqual(bundle_obj.patches.count(), > > + len(bundle_json['patches'])) > > + for patch_obj, patch_json in zip( > > + bundle_obj.patches.all(), bundle_json['patches']): > > + self.assertEqual(patch_obj.id, patch_json['id']) > > > > def test_list_empty(self): > > """List bundles when none are present.""" > > @@ -179,18 +184,117 @@ class TestBundleAPI(utils.APITestCase): > > self.assertIn('url', resp.data) > > self.assertNotIn('web_url', resp.data) > > > > - def test_create_update_delete(self): > > - """Ensure creates, updates and deletes aren't allowed""" > > + def _test_create_update(self, authenticate=True): > > + user = create_user() > > + project = create_project() > > + patch_a = create_patch(project=project) > > + patch_b = create_patch(project=project) > > + > > + if authenticate: > > + self.client.force_authenticate(user=user) > > + > > + return user, project, patch_a, patch_b > > + > > + @utils.store_samples('bundle-create-error-forbidden') > > + def test_create_anonymous(self): > > + """Create a bundle when not signed in. > > + > > + Ensure creations can only be performed by signed in users. > > + """ > > + user, project, patch_a, patch_b = self._test_create_update( > > + authenticate=False) > > + bundle = { > > + 'name': 'test-bundle', > > + 'public': True, > > + 'patches': [patch_a.id, patch_b.id], > > + } > > + > > + resp = self.client.post(self.api_url(), bundle) > > + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code) > > + > > + @utils.store_samples('bundle-create') > > + def test_create(self): > > + """Validate we can create a new bundle.""" > > + user, project, patch_a, patch_b = self._test_create_update() > > + bundle = { > > + 'name': 'test-bundle', > > + 'public': True, > > + 'patches': [patch_a.id, patch_b.id], > > + } > > + > > + resp = self.client.post(self.api_url(), bundle) > > + self.assertEqual(status.HTTP_201_CREATED, resp.status_code) > > + self.assertEqual(1, Bundle.objects.all().count()) > > + self.assertSerialized(Bundle.objects.first(), resp.data) > > + > > + @utils.store_samples('bundle-update-not-found') > > + def test_update_anonymous(self): > > + """Update an existing bundle when not signed in. > > + > > + Ensure updates can only be performed by signed in users. > > + """ > > + user, project, patch_a, patch_b = self._test_create_update( > > + authenticate=False) > > + bundle = create_bundle(owner=user, project=project) > > + > > + resp = self.client.patch(self.api_url(bundle.id), { > > + 'name': 'hello-bundle', 'patches': [patch_a.id, patch_b.id]}) > > + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code) > > + > > + @utils.store_samples('bundle-update') > > + def test_update(self): > > + """Validate we can update an existing bundle.""" > > + user, project, patch_a, patch_b = self._test_create_update() > > + bundle = create_bundle(owner=user, project=project) > > + > > + resp = self.client.patch(self.api_url(bundle.id), { > > + 'name': 'hello-bundle', 'patches': [patch_a.id, patch_b.id]}) > > + self.assertEqual(status.HTTP_200_OK, resp.status_code) > > + self.assertEqual(1, Bundle.objects.all().count()) > > + self.assertEqual(len(resp.data['patches']), 2) > > + self.assertEqual(resp.data['name'], 'hello-bundle') > > + > > + @utils.store_samples('bundle-delete-not-found') > > + def test_delete_anonymous(self): > > + """Delete a bundle when not signed in. > > + > > + Ensure deletions can only be performed when signed in. > > + """ > > + user, project, patch_a, patch_b = self._test_create_update( > > + authenticate=False) > > + bundle = create_bundle(owner=user, project=project) > > + > > + resp = self.client.delete(self.api_url(bundle.id)) > > + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code) > > + > > + @utils.store_samples('bundle-delete') > > + def test_delete(self): > > + """Validate we can delete an existing bundle.""" > > + user = create_user() > > + bundle = create_bundle(owner=user) > > + > > + self.client.force_authenticate(user=user) > > + > > + resp = self.client.delete(self.api_url(bundle.id)) > > + self.assertEqual(status.HTTP_204_NO_CONTENT, resp.status_code) > > + self.assertEqual(0, Bundle.objects.all().count()) > > + > > + def test_create_update_delete_version_1_1(self): > > + """Ensure creates, updates and deletes aren't allowed with old > > API.""" > > user = create_maintainer() > > user.is_superuser = True > > user.save() > > self.client.force_authenticate(user=user) > > > > - resp = self.client.post(self.api_url(), {'email': 'f...@f.com'}) > > + resp = self.client.post(self.api_url(version='1.1'), {'name': > > 'test'}, > > + validate_schema=False) > > self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, > > resp.status_code) > > > > - resp = self.client.patch(self.api_url(user.id), {'email': > > 'f...@f.com'}) > > + resp = self.client.patch(self.api_url(1, version='1.1'), > > + {'name': 'test'}, > > + validate_schema=False) > > self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, > > resp.status_code) > > > > - resp = self.client.delete(self.api_url(1)) > > + resp = self.client.delete(self.api_url(1, version='1.1'), > > + validate_schema=False) > > self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, > > resp.status_code) > > diff --git a/patchwork/tests/api/utils.py b/patchwork/tests/api/utils.py > > index 0c232d04..ce83ce2b 100644 > > --- a/patchwork/tests/api/utils.py > > +++ b/patchwork/tests/api/utils.py > > @@ -112,44 +112,52 @@ class APIClient(BaseAPIClient): > > self.factory = APIRequestFactory() > > > > def get(self, path, data=None, follow=False, **extra): > > + validate_schema = extra.pop('validate_schema', True) > > request = self.factory.get( > > path, data=data, SERVER_NAME='example.com', **extra) > > response = super(APIClient, self).get( > > path, data=data, follow=follow, SERVER_NAME='example.com', > > **extra) > > - validator.validate_data(path, request, response) > > + if validate_schema: > > + validator.validate_data(path, request, response) > > return response > > > > def post(self, path, data=None, format=None, content_type=None, > > follow=False, **extra): > > + validate_schema = extra.pop('validate_schema', True) > > request = self.factory.post( > > path, data=data, format='json', content_type=content_type, > > SERVER_NAME='example.com', **extra) > > response = super(APIClient, self).post( > > path, data=data, format='json', content_type=content_type, > > follow=follow, SERVER_NAME='example.com', **extra) > > - validator.validate_data(path, request, response) > > + if validate_schema: > > + validator.validate_data(path, request, response) > > return response > > > > def put(self, path, data=None, format=None, content_type=None, > > follow=False, **extra): > > + validate_schema = extra.pop('validate_schema', True) > > request = self.factory.put( > > path, data=data, format='json', content_type=content_type, > > SERVER_NAME='example.com', **extra) > > response = super(APIClient, self).put( > > path, data=data, format='json', content_type=content_type, > > follow=follow, SERVER_NAME='example.com', **extra) > > - validator.validate_data(path, request, response) > > + if validate_schema: > > + validator.validate_data(path, request, response) > > return response > > > > def patch(self, path, data=None, format=None, content_type=None, > > follow=False, **extra): > > + validate_schema = extra.pop('validate_schema', True) > > request = self.factory.patch( > > path, data=data, format='json', content_type=content_type, > > SERVER_NAME='example.com', **extra) > > response = super(APIClient, self).patch( > > path, data=data, format='json', content_type=content_type, > > follow=follow, SERVER_NAME='example.com', **extra) > > - validator.validate_data(path, request, response) > > + if validate_schema: > > + validator.validate_data(path, request, response) > > return response > > > > > > diff --git a/releasenotes/notes/update-bundle-via-api-2946d8c4e730d545.yaml > > b/releasenotes/notes/update-bundle-via-api-2946d8c4e730d545.yaml > > new file mode 100644 > > index 00000000..bfa1ef55 > > --- /dev/null > > +++ b/releasenotes/notes/update-bundle-via-api-2946d8c4e730d545.yaml > > @@ -0,0 +1,4 @@ > > +--- > > +api: > > + - | > > + Bundles can now be created, updated and deleted via the REST API. > > -- > > 2.21.0 _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork