----- Original Message ----- > From: "Stephen Finucane" <step...@that.guru> > To: vkaba...@redhat.com, patchwork@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, vkaba...@redhat.com wrote: > > From: Veronika Kabatova <vkaba...@redhat.com> > > > > Signed-off-by: Veronika Kabatova <vkaba...@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 > > + > > + > > +@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')) > > + > > + > > +@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 > > _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork