[PATCH v4] api: Add comments to patch and cover endpoints
Veronika Kabatova
vkabatov at redhat.com
Sat Apr 28 01:57:10 AEST 2018
----- Original Message -----
> From: "Stephen Finucane" <stephen at that.guru>
> To: vkabatov at redhat.com, patchwork at lists.ozlabs.org
> Sent: Friday, April 27, 2018 5:37:57 PM
> Subject: Re: [PATCH v4] api: Add comments to patch and cover endpoints
>
> On Wed, 2018-04-25 at 19:33 +0200, vkabatov at redhat.com wrote:
> > From: Veronika Kabatova <vkabatov at redhat.com>
> >
> > Signed-off-by: Veronika Kabatova <vkabatov at redhat.com>
>
> Two comments below. I've actually addressed these already with my own
> patch. Assuming you're happy with said patch, I can squash it with this
> one and merge them.
>
Either way works for me, and your follow-up patch looks good to me. I'll
send my ack right away.
Thanks,
Veronika
> Cheers,
> Stephen
>
> > ---
> > patchwork/api/comment.py | 75 ++++++++++++++
> > patchwork/api/cover.py | 13 ++-
> > patchwork/api/patch.py | 16 ++-
> > patchwork/models.py | 3 +
> > patchwork/tests/api/test_comment.py | 115
> > +++++++++++++++++++++
> > patchwork/tests/api/test_cover.py | 11 +-
> > patchwork/tests/api/test_patch.py | 19 +++-
> > patchwork/urls.py | 11 ++
> > .../notes/comments-api-b7dff6ee4ce04c9b.yaml | 8 ++
> > 9 files changed, 262 insertions(+), 9 deletions(-)
> > create mode 100644 patchwork/api/comment.py
> > create mode 100644 patchwork/tests/api/test_comment.py
> > create mode 100644 releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml
> >
> > diff --git a/patchwork/api/comment.py b/patchwork/api/comment.py
> > new file mode 100644
> > index 0000000..fbb7cc8
> > --- /dev/null
> > +++ b/patchwork/api/comment.py
> > @@ -0,0 +1,75 @@
> > +# Patchwork - automated patch tracking system
> > +# Copyright (C) 2018 Red Hat
> > +#
> > +# 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 email.parser
> > +
> > +from rest_framework.generics import ListAPIView
> > +from rest_framework.serializers import SerializerMethodField
> > +
> > +from patchwork.api.base import BaseHyperlinkedModelSerializer
> > +from patchwork.api.base import PatchworkPermission
> > +from patchwork.api.embedded import PersonSerializer
> > +from patchwork.models import Comment
> > +
> > +
> > +class CommentListSerializer(BaseHyperlinkedModelSerializer):
> > +
> > + subject = SerializerMethodField()
> > + headers = SerializerMethodField()
> > + submitter = PersonSerializer(read_only=True)
> > +
> > + def get_subject(self, comment):
> > + return email.parser.Parser().parsestr(comment.headers,
> > + True).get('Subject', '')
> > +
> > + def get_headers(self, comment):
> > + headers = {}
> > +
> > + if comment.headers:
> > + parsed = email.parser.Parser().parsestr(comment.headers, True)
> > + for key in parsed.keys():
> > + headers[key] = parsed.get_all(key)
> > + # Let's return a single string instead of a list if only
> > one
> > + # header with this key is present
> > + if len(headers[key]) == 1:
> > + headers[key] = headers[key][0]
> > +
> > + return headers
> > +
> > + class Meta:
> > + model = Comment
> > + fields = ('id', 'msgid', 'date', 'subject', 'submitter',
> > 'content',
> > + 'headers')
> > + read_only_fields = fields
> > +
> > +
> > +class CommentList(ListAPIView):
> > + """List comments"""
> > +
> > + permission_classes = (PatchworkPermission,)
> > + serializer_class = CommentListSerializer
> > + search_fields = ('subject',)
> > + ordering_fields = ('id', 'subject', 'date', 'submitter')
> > + ordering = 'id'
> > + lookup_url_kwarg = 'pk'
> > +
> > + def get_queryset(self):
> > + return Comment.objects.filter(
> > + submission=self.kwargs['pk']
> > + ).select_related('submitter')
> > diff --git a/patchwork/api/cover.py b/patchwork/api/cover.py
> > index fc7ae97..19f0f65 100644
> > --- a/patchwork/api/cover.py
> > +++ b/patchwork/api/cover.py
> > @@ -21,6 +21,7 @@ import email.parser
> >
> > from rest_framework.generics import ListAPIView
> > from rest_framework.generics import RetrieveAPIView
> > +from rest_framework.reverse import reverse
> > from rest_framework.serializers import SerializerMethodField
> >
> > from patchwork.api.base import BaseHyperlinkedModelSerializer
> > @@ -58,6 +59,11 @@ class
> > CoverLetterListSerializer(BaseHyperlinkedModelSerializer):
> > class CoverLetterDetailSerializer(CoverLetterListSerializer):
>
> Any reason we don't include these in the list resources? Seems like a
> sensible thing to include.
>
> >
> > headers = SerializerMethodField()
> > + comments = SerializerMethodField()
> > +
> > + def get_comments(self, cover):
> > + return self.context.get('request').build_absolute_uri(
> > + reverse('api-comment-list', kwargs={'pk': cover.id}))
> >
> > def get_headers(self, instance):
> > if instance.headers:
> > @@ -65,9 +71,14 @@ class
> > CoverLetterDetailSerializer(CoverLetterListSerializer):
> >
> > class Meta:
> > model = CoverLetter
> > - fields = CoverLetterListSerializer.Meta.fields + ('headers',
> > 'content')
> > + fields = CoverLetterListSerializer.Meta.fields + ('headers',
> > + 'content',
> > + 'comments')
> > read_only_fields = fields
> > extra_kwargs = CoverLetterListSerializer.Meta.extra_kwargs
> > + versioned_fields = {
> > + '1.1': ('mbox', 'comments'),
> > + }
> >
> >
> > class CoverLetterList(ListAPIView):
> > diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
> > index 115feff..feb7d80 100644
> > --- a/patchwork/api/patch.py
> > +++ b/patchwork/api/patch.py
> > @@ -24,9 +24,9 @@ from rest_framework.generics import ListAPIView
> > from rest_framework.generics import RetrieveUpdateAPIView
> > from rest_framework.relations import RelatedField
> > from rest_framework.reverse import reverse
> > -from rest_framework.serializers import HyperlinkedModelSerializer
> > from rest_framework.serializers import SerializerMethodField
> >
> > +from patchwork.api.base import BaseHyperlinkedModelSerializer
> > from patchwork.api.base import PatchworkPermission
> > from patchwork.api.filters import PatchFilter
> > from patchwork.api.embedded import PersonSerializer
> > @@ -75,7 +75,7 @@ class StateField(RelatedField):
> > return State.objects.all()
> >
> >
> > -class PatchListSerializer(HyperlinkedModelSerializer):
> > +class PatchListSerializer(BaseHyperlinkedModelSerializer):
> >
> > project = ProjectSerializer(read_only=True)
> > state = StateField()
> > @@ -121,6 +121,11 @@ class PatchDetailSerializer(PatchListSerializer):
>
> Ditto.
>
> > headers = SerializerMethodField()
> > prefixes = SerializerMethodField()
> > + comments = SerializerMethodField()
> > +
> > + def get_comments(self, patch):
> > + return self.context.get('request').build_absolute_uri(
> > + reverse('api-comment-list', kwargs={'pk': patch.id}))
> >
> > def get_headers(self, patch):
> > if patch.headers:
> > @@ -132,10 +137,13 @@ class PatchDetailSerializer(PatchListSerializer):
> > class Meta:
> > model = Patch
> > fields = PatchListSerializer.Meta.fields + (
> > - 'headers', 'content', 'diff', 'prefixes')
> > + 'headers', 'content', 'diff', 'prefixes', 'comments')
> > read_only_fields = PatchListSerializer.Meta.read_only_fields + (
> > - 'headers', 'content', 'diff', 'prefixes')
> > + 'headers', 'content', 'diff', 'prefixes', 'comments')
> > extra_kwargs = PatchListSerializer.Meta.extra_kwargs
> > + versioned_fields = {
> > + '1.1': ('comments', ),
> > + }
> >
> >
> > class PatchList(ListAPIView):
> > diff --git a/patchwork/models.py b/patchwork/models.py
> > index f91b994..67c2d3a 100644
> > --- a/patchwork/models.py
> > +++ b/patchwork/models.py
> > @@ -613,6 +613,9 @@ class Comment(EmailMixin, models.Model):
> > if hasattr(self.submission, 'patch'):
> > self.submission.patch.refresh_tag_counts()
> >
> > + def is_editable(self, user):
> > + return False
> > +
> > class Meta:
> > ordering = ['date']
> > unique_together = [('msgid', 'submission')]
> > diff --git a/patchwork/tests/api/test_comment.py
> > b/patchwork/tests/api/test_comment.py
> > new file mode 100644
> > index 0000000..b283bfe
> > --- /dev/null
> > +++ b/patchwork/tests/api/test_comment.py
> > @@ -0,0 +1,115 @@
> > +# Patchwork - automated patch tracking system
> > +# Copyright (C) 2018 Red Hat
> > +#
> > +# 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 patchwork.compat import NoReverseMatch
> > +from patchwork.compat import reverse
> > +from patchwork.tests.utils import create_comment
> > +from patchwork.tests.utils import create_cover
> > +from patchwork.tests.utils import create_patch
> > +from patchwork.tests.utils import SAMPLE_CONTENT
> > +
> > +if settings.ENABLE_REST_API:
> > + from rest_framework import status
> > + from rest_framework.test import APITestCase
> > +else:
> > + # stub out APITestCase
> > + from django.test import TestCase
> > + APITestCase = TestCase # noqa
> > +
> > +
> > + at unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
> > +class TestCoverComments(APITestCase):
> > + @staticmethod
> > + def api_url(cover, version=None):
> > + kwargs = {}
> > + if version:
> > + kwargs['version'] = version
> > + kwargs['pk'] = cover.id
> > +
> > + return reverse('api-comment-list', kwargs=kwargs)
> > +
> > + def assertSerialized(self, comment_obj, comment_json):
> > + self.assertEqual(comment_obj.id, comment_json['id'])
> > + self.assertEqual(comment_obj.submitter.id,
> > + comment_json['submitter']['id'])
> > + self.assertIn(SAMPLE_CONTENT, comment_json['content'])
> > +
> > + def test_list(self):
> > + cover_obj = create_cover()
> > + resp = self.client.get(self.api_url(cover_obj))
> > + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> > + self.assertEqual(0, len(resp.data))
> > +
> > + comment_obj = create_comment(submission=cover_obj)
> > + resp = self.client.get(self.api_url(cover_obj))
> > + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> > + self.assertEqual(1, len(resp.data))
> > + self.assertSerialized(comment_obj, resp.data[0])
> > +
> > + another_comment = create_comment(submission=cover_obj)
> > + resp = self.client.get(self.api_url(cover_obj))
> > + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> > + self.assertEqual(2, len(resp.data))
> > +
> > + # check we can't access comments using the old version of the API
> > + with self.assertRaises(NoReverseMatch):
> > + self.client.get(self.api_url(cover_obj, version='1.0'))
> > +
> > +
> > + at unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
> > +class TestPatchComments(APITestCase):
> > + @staticmethod
> > + def api_url(patch, version=None):
> > + kwargs = {}
> > + if version:
> > + kwargs['version'] = version
> > + kwargs['pk'] = patch.id
> > +
> > + return reverse('api-comment-list', kwargs=kwargs)
> > +
> > + def assertSerialized(self, comment_obj, comment_json):
> > + self.assertEqual(comment_obj.id, comment_json['id'])
> > + self.assertEqual(comment_obj.submitter.id,
> > + comment_json['submitter']['id'])
> > + self.assertIn(SAMPLE_CONTENT, comment_json['content'])
> > +
> > + def test_list(self):
> > + patch_obj = create_patch()
> > + resp = self.client.get(self.api_url(patch_obj))
> > + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> > + self.assertEqual(0, len(resp.data))
> > +
> > + comment_obj = create_comment(submission=patch_obj)
> > + resp = self.client.get(self.api_url(patch_obj))
> > + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> > + self.assertEqual(1, len(resp.data))
> > + self.assertSerialized(comment_obj, resp.data[0])
> > +
> > + another_comment = create_comment(submission=patch_obj)
> > + resp = self.client.get(self.api_url(patch_obj))
> > + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> > + self.assertEqual(2, len(resp.data))
> > +
> > + # check we can't access comments using the old version of the API
> > + with self.assertRaises(NoReverseMatch):
> > + self.client.get(self.api_url(patch_obj, version='1.0'))
> > diff --git a/patchwork/tests/api/test_cover.py
> > b/patchwork/tests/api/test_cover.py
> > index 3135b7e..9a0fbbb 100644
> > --- a/patchwork/tests/api/test_cover.py
> > +++ b/patchwork/tests/api/test_cover.py
> > @@ -49,7 +49,8 @@ class TestCoverLetterAPI(APITestCase):
> >
> > if item is None:
> > return reverse('api-cover-list', kwargs=kwargs)
> > - return reverse('api-cover-detail', args=[item], kwargs=kwargs)
> > + kwargs['pk'] = item
> > + return reverse('api-cover-detail', kwargs=kwargs)
> >
> > def assertSerialized(self, cover_obj, cover_json):
> > self.assertEqual(cover_obj.id, cover_json['id'])
> > @@ -115,6 +116,14 @@ class TestCoverLetterAPI(APITestCase):
> > self.assertEqual(status.HTTP_200_OK, resp.status_code)
> > self.assertSerialized(cover_obj, resp.data)
> >
> > + # test comments
> > + resp = self.client.get(self.api_url(cover_obj.id))
> > + self.assertIn('comments', resp.data)
> > +
> > + # test old version of API
> > + resp = self.client.get(self.api_url(cover_obj.id, version='1.0'))
> > + self.assertNotIn('comments', resp.data)
> > +
> > def test_create_update_delete(self):
> > user = create_maintainer()
> > user.is_superuser = True
> > diff --git a/patchwork/tests/api/test_patch.py
> > b/patchwork/tests/api/test_patch.py
> > index 909c1eb..d9d44d3 100644
> > --- a/patchwork/tests/api/test_patch.py
> > +++ b/patchwork/tests/api/test_patch.py
> > @@ -45,10 +45,15 @@ class TestPatchAPI(APITestCase):
> > fixtures = ['default_tags']
> >
> > @staticmethod
> > - def api_url(item=None):
> > + def api_url(item=None, version=None):
> > + kwargs = {}
> > + if version:
> > + kwargs['version'] = version
> > +
> > if item is None:
> > - return reverse('api-patch-list')
> > - return reverse('api-patch-detail', args=[item])
> > + return reverse('api-patch-list', kwargs=kwargs)
> > + kwargs['pk'] = item
> > + return reverse('api-patch-detail', kwargs=kwargs)
> >
> > def assertSerialized(self, patch_obj, patch_json):
> > self.assertEqual(patch_obj.id, patch_json['id'])
> > @@ -130,6 +135,14 @@ class TestPatchAPI(APITestCase):
> > self.assertEqual(patch.diff, resp.data['diff'])
> > self.assertEqual(0, len(resp.data['tags']))
> >
> > + # test comments
> > + resp = self.client.get(self.api_url(patch.id))
> > + self.assertIn('comments', resp.data)
> > +
> > + # test old version of API
> > + resp = self.client.get(self.api_url(item=patch.id, version='1.0'))
> > + self.assertNotIn('comments', resp.data)
> > +
> > def test_create(self):
> > """Ensure creations are rejected."""
> > project = create_project()
> > diff --git a/patchwork/urls.py b/patchwork/urls.py
> > index 0893fe2..1dc4ffc 100644
> > --- a/patchwork/urls.py
> > +++ b/patchwork/urls.py
> > @@ -213,6 +213,7 @@ if settings.ENABLE_REST_API:
> >
> > from patchwork.api import bundle as api_bundle_views # noqa
> > from patchwork.api import check as api_check_views # noqa
> > + from patchwork.api import comment as api_comment_views # noqa
> > from patchwork.api import cover as api_cover_views # noqa
> > from patchwork.api import event as api_event_views # noqa
> > from patchwork.api import index as api_index_views # noqa
> > @@ -279,8 +280,18 @@ if settings.ENABLE_REST_API:
> > name='api-event-list'),
> > ]
> >
> > + api_1_1_patterns = [
> > + url(r'^patches/(?P<pk>[^/]+)/comments/$',
> > + api_comment_views.CommentList.as_view(),
> > + name='api-comment-list'),
> > + url(r'^covers/(?P<pk>[^/]+)/comments/$',
> > + api_comment_views.CommentList.as_view(),
> > + name='api-comment-list'),
> > + ]
> > +
> > urlpatterns += [
> > url(r'^api/(?:(?P<version>(1.0|1.1))/)?', include(api_patterns)),
> > + url(r'^api/(?:(?P<version>1.1)/)?', include(api_1_1_patterns)),
> >
> > # token change
> > url(r'^user/generate-token/$', user_views.generate_token,
> > diff --git a/releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml
> > b/releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml
> > new file mode 100644
> > index 0000000..6cd6441
> > --- /dev/null
> > +++ b/releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml
> > @@ -0,0 +1,8 @@
> > +---
> > +api:
> > + - |
> > + Links to related comments are now exposed when checking patch and
> > cover
> > + letter details. The comments themselves are then available via
> > + ``/patches/{patchID}/comments`` and ``/covers/{coverID}/comments``
> > + endpoints. Please note that comments are available only since API
> > + version 1.1
>
>
More information about the Patchwork
mailing list